Add Solana priority fees #23214

nvonpentz · 2024-04-22T19:01:07Z

Resolves brave/brave-browser#35866

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

brave-builds · 2024-04-29T23:37:15Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-04-30T16:49:25Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-02T14:43:53Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-02T14:46:29Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-02T22:18:23Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-03T00:49:22Z

A Storybook has been deployed to preview UI for the latest push

josheleonard

desktop frontend ++

brave-builds · 2024-05-03T14:57:40Z

A Storybook has been deployed to preview UI for the latest push

StephenHeaps

iOS ++

yrliou · 2024-04-22T19:37:15Z

components/brave_wallet/common/brave_wallet.mojom

@@ -2382,6 +2392,8 @@ struct SolanaTxData {
  // signAllTransactions provider APIs, which includes serialized message and
  // signatures from partial_sign.
  SolanaSignTransactionParam? sign_transaction_param;
+
+  SolanaGasEstimation? gas_estimation;


nit: let's use fee estimation instead.

yrliou · 2024-04-22T19:39:31Z

components/brave_wallet/browser/solana_tx_manager.h

@@ -20,6 +20,9 @@ class PrefService;

 namespace brave_wallet {

+// https://docs.rs/solana-program/1.18.10/src/solana_program/clock.rs.html#129-131
+extern const int kValidBlockHeightThreshold;


Let's move the constant to a shared file rather than use extern here, and rename to kSolanaValidBlockHeightThreshold if needed.

yrliou · 2024-04-22T19:41:56Z

components/brave_wallet/browser/solana_transaction.h

@@ -155,6 +161,9 @@ class SolanaTransaction {
  // Currently might be specified by solana.signAndSendTransaction provider
  // API as the options to be passed to sendTransaction RPC call.
  std::optional<SendOptions> send_options_;
+
+  // Gas estimation result
+  mojom::SolanaGasEstimationPtr gas_estimation_;


add a check in SolanaTransaction::operator==

This was addressed after you wrote your comment but before it was posted to the pull request.

yrliou · 2024-05-07T17:09:20Z

components/brave_wallet/common/brave_wallet.mojom

+  GetSolanaGasEstimation(string chain_id, string tx_meta_id)
+    => (SolanaGasEstimation estimation, SolanaProviderError error, string error_message);


Why do we need two instead of just changing fee to the new struct in GetEstimatedTxFee?

yrliou · 2024-05-07T17:31:10Z

components/brave_wallet/browser/solana_message.cc

+  // Rearrange the static account keys and generate a new SolanaMessageHeader
+  uint16_t num_required_signatures = 0;
+  uint16_t num_readonly_signed_accounts = 0;
+  uint16_t num_readonly_unsigned_accounts = 0;
+  std::vector<SolanaAccountMeta> unique_account_metas;
+  GetUniqueAccountMetas(fee_payer_, instructions_, &unique_account_metas);
+  std::vector<SolanaAddress> static_accounts;
+  for (const auto& meta : unique_account_metas) {
+    auto addr = SolanaAddress::FromBase58(meta.pubkey);
+    if (!addr) {
+      return false;
+    }
+
+    if (meta.is_signer) {
+      num_required_signatures++;
+    }
+    if (meta.is_signer && !meta.is_writable) {
+      num_readonly_signed_accounts++;
+    }
+    if (!meta.is_signer && !meta.is_writable) {
+      num_readonly_unsigned_accounts++;
+    }
+
+    if (num_required_signatures > UINT8_MAX ||
+        num_readonly_signed_accounts > UINT8_MAX ||
+        num_readonly_unsigned_accounts > UINT8_MAX ||
+        static_accounts.size() == UINT8_MAX) {
+      return false;
+    }
+    static_accounts.emplace_back(*addr);
+  }
+  static_account_keys_ = static_accounts;
+


We should extract and share code in CreateLegacyMessage rather than copy the logic.

brave-builds · 2024-05-29T12:41:18Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-29T14:06:04Z

A Storybook has been deployed to preview UI for the latest push

…wser test

* Fix to support AddPriorityFEe for v0 transactions * Test case for AddPriorityFee for v0 transactions * Test case for UsesPriorityFee

* If fetching the base fee fails, don't set a gas estimate or modify the priority fee instructions, but still add the transaction. The client will need to fetch the estimate again on it's own as it currently does. * If only setting the priority fee fails (e.g. simulation), we set the gas estimate, but don't modify the instructions. We do this because the frontend can still use base fee from the estimate, but we don't have enough info to set the priority fee instruction.

…tend

github-actions · 2024-05-30T13:39:31Z

[puLL-Merge] - brave/brave-core@23214

Here is a review of the pull request:

Description

This PR makes several changes to improve Solana transaction handling in Brave Wallet:

Adds support for fetching fee estimations for Solana transactions, including simulating transactions to get compute unit usage estimates and fetching recent prioritization fees.
Updates the Solana transaction manager to automatically add compute budget instructions and priority fee to transactions when possible.
Propagates the fee estimation results through the UI.

The motivation seems to be to provide more accurate and complete fee estimations for Solana transactions and enable users to add priority fees.

Changes

Changes

Changes are organized by area:

Core:

brave_wallet_constants.h: Adds some new Solana-related constants
json_rpc_service.cc/h: Implements new methods to simulate Solana transactions and get recent prioritization fees
solana_instruction_builder.cc/h: Adds functions to create Solana compute budget program instructions
solana_instruction_data_decoder.cc/h: Adds function to get the type of a compute budget instruction
solana_message.cc/h: Updates to support adding priority fee instructions to a Solana message
solana_requests.cc/h: Adds requests to simulate a transaction and get recent prioritization fees
solana_response_parser.cc/h: Adds functions to parse responses for simulating transactions and getting recent fees
solana_transaction.cc/h: Updates serialization to include fee estimation results
solana_tx_manager.cc/h: Major changes to fetch fee estimations when adding unapproved transactions. Will automatically add compute budget instructions and priority fee if possible.

Tests:

brave_wallet/browser/*_unittest.cc files: Adds unit tests for the new functionality
brave_wallet_p3a_unittest.cc: Updates test expectations
wallet_button_notification_source_browsertest.cc: Makes test transaction more realistic

UI:

Updates mock bridge data to match new fee estimation format
Updates transaction confirmation to calculate total fee from base fee and priority fee
Updates stores and extensions to use new solanaTxFeeEstimation method

Security Hotspots

No major security issues identified. The changes primarily impact already-existing Solana transaction handling rather than opening up new attack surface.

One note is that user-provided compute unit limits in transactions are now respected. However, this seems to be the intended behavior, and there are still limits in place (max transaction size, etc).

Let me know if you have any other questions!

brave-builds · 2024-05-30T14:36:19Z

A Storybook has been deployed to preview UI for the latest push

nvonpentz force-pushed the solana-priority-fees branch from 1fcb940 to ceaeaf4 Compare April 22, 2024 20:13

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Apr 22, 2024

nvonpentz force-pushed the solana-priority-fees branch 3 times, most recently from d9d87c2 to a9c030f Compare April 29, 2024 22:58

nvonpentz force-pushed the solana-priority-fees branch from a9c030f to 55a0dcc Compare April 30, 2024 16:08

nvonpentz marked this pull request as ready for review May 2, 2024 14:09

nvonpentz requested review from a team as code owners May 2, 2024 14:09

github-actions bot added the puLL-Merge label May 2, 2024

nvonpentz force-pushed the solana-priority-fees branch from 2ea81ad to 0e37933 Compare May 3, 2024 00:09

josheleonard approved these changes May 3, 2024

View reviewed changes

nvonpentz force-pushed the solana-priority-fees branch from 0e37933 to 621277c Compare May 3, 2024 14:19

StephenHeaps approved these changes May 3, 2024

View reviewed changes

yrliou assigned nvonpentz May 6, 2024

yrliou reviewed May 7, 2024

View reviewed changes

nvonpentz force-pushed the solana-priority-fees branch from 621277c to 7552175 Compare May 14, 2024 00:12

nvonpentz marked this pull request as draft May 14, 2024 00:12

nvonpentz force-pushed the solana-priority-fees branch from 10a6041 to 1a0851d Compare May 29, 2024 12:00

nvonpentz force-pushed the solana-priority-fees branch from 1a0851d to 3b68864 Compare May 29, 2024 13:25

nvonpentz added 23 commits May 30, 2024 09:30

Add Solana priority fees

9057469

Fix AddAndApproveTransaction test

ea03ab6

Fix WalletButtonNotificationSourceTest.PendingTransactionsCounter bro…

5f34e04

…wser test

Fix SolanaProviderTest.* browser tests

1cdf53a

Fix story book mock data

31fb15c

Add gas estimation to iOS mock tx data

aa33abf

Use a default minimum value for priority fee of 1 micro lamport

4b0886d

Fix windows test failures

bec591e

Use confirmed commitment level for getFeeForMessage

8c29749

Do not add priority fee if there already is one

9a0a2c1

Add v0 support for AddPriorityFee + tests

f7973d9

* Fix to support AddPriorityFEe for v0 transactions * Test case for AddPriorityFee for v0 transactions * Test case for UsesPriorityFee

Add missing instruction builder tests and remove some TODOs

f8800ba

Presubmit fix

efd0b6f

Rename kSolanaValidBlockHeightThreshold and move to shared file

85aa57a

Add empty data test case to GetComputeBudgetInstructionType test

859725e

Extract shared code into ProcessAccountMetas function

56aa2e6

Rename SolanaGasEstimation to SolanaFeeEstimate

694e6df

Merge existing solana base fee getter with priority fee getter

7815e99

Fix iOS interfaces

85479da

Address feedback

a2a2293

Calculate priority fee and include in total fee estimate used by fron…

1d4688d

…tend

Try to format iOS

97fb9e7

nvonpentz force-pushed the solana-priority-fees branch from 3b68864 to 97fb9e7 Compare May 30, 2024 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Solana priority fees #23214

Add Solana priority fees #23214

nvonpentz commented Apr 22, 2024 •

edited

brave-builds commented Apr 29, 2024

brave-builds commented Apr 30, 2024

brave-builds commented May 2, 2024

brave-builds commented May 2, 2024

brave-builds commented May 2, 2024

brave-builds commented May 3, 2024

josheleonard left a comment

brave-builds commented May 3, 2024

StephenHeaps left a comment

yrliou Apr 22, 2024

yrliou Apr 22, 2024

yrliou Apr 22, 2024

nvonpentz May 13, 2024

yrliou May 7, 2024

yrliou May 7, 2024

brave-builds commented May 29, 2024

brave-builds commented May 29, 2024

github-actions bot commented May 30, 2024

Changes

brave-builds commented May 30, 2024

		GetSolanaGasEstimation(string chain_id, string tx_meta_id)
		=> (SolanaGasEstimation estimation, SolanaProviderError error, string error_message);

Add Solana priority fees #23214

Are you sure you want to change the base?

Add Solana priority fees #23214

Conversation

nvonpentz commented Apr 22, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Apr 29, 2024

brave-builds commented Apr 30, 2024

brave-builds commented May 2, 2024

brave-builds commented May 2, 2024

brave-builds commented May 2, 2024

brave-builds commented May 3, 2024

josheleonard left a comment

Choose a reason for hiding this comment

brave-builds commented May 3, 2024

StephenHeaps left a comment

Choose a reason for hiding this comment

yrliou Apr 22, 2024

Choose a reason for hiding this comment

yrliou Apr 22, 2024

Choose a reason for hiding this comment

yrliou Apr 22, 2024

Choose a reason for hiding this comment

nvonpentz May 13, 2024

Choose a reason for hiding this comment

yrliou May 7, 2024

Choose a reason for hiding this comment

yrliou May 7, 2024

Choose a reason for hiding this comment

brave-builds commented May 29, 2024

brave-builds commented May 29, 2024

github-actions bot commented May 30, 2024

Description

Changes

Security Hotspots

brave-builds commented May 30, 2024

nvonpentz commented Apr 22, 2024 •

edited