Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(wallet): migrate to Blowfish API v2023-06-05 #21609

Merged
merged 6 commits into from
Jan 19, 2024

Conversation

onyb
Copy link
Member

@onyb onyb commented Jan 18, 2024

Resolves brave/brave-browser#34619

EVM docs https://docs.blowfish.xyz/reference/scan-transactions-evm
Solana docs https://docs.blowfish.xyz/reference/scan-transactions-solana

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 lint, 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:

Test Plan:

N/A

Copy link
Contributor

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

Description

This PR includes changes for updating the constant kBlowfishAPIVersion within the Brave Wallet component to a newer timestamp value, and it modifies the EncodeScanTransactionParams functions and their respective tests to handle a new data structure.

The updates in simulation_request_helper.cc seem to be focused on refactoring the functions responsible for building the transaction scan simulation request payloads for EVM and Solana transactions, updating returned values to include additional information (i.e. userAccount) in a pair, and adjusting the unit tests accordingly.

The rationale for these changes could include meeting new requirements from the simulation API endpoint (possibly for Blowfish, a service that scans transactions for security purposes), or improving the functionalities by including more relevant data in the transaction simulation request.

Changes

Changes

Changes are organized by filename.

brave_wallet/browser/simulation_request_helper.cc

  • Modified EncodeScanTransactionParams function signatures to return a pair with the second element being the user account.
  • Updated function bodies to handle new structure and include user account in the return value.

brave_wallet/browser/simulation_request_helper.h

  • Reflected changes in function declarations to match updates in the .cc file.

brave_wallet/browser/simulation_request_helper_unittest.cc

  • Adjusted unit tests to validate the updated functions which now return a pair containing the user account.

brave_wallet/browser/simulation_response_parser.cc

  • Refactored the ParseSimulationResponse functions, taking into account new structures and potentially more reliable parsing.

brave_wallet/browser/simulation_response_parser.h

  • Updated the function declarations to match the new parsing logic.

Security Hotspots

  1. brave_wallet/browser/simulation_request_helper.cc & brave_wallet/browser/simulation_request_helper_unittest.cc:

    • Ensure that returning the user account as part of responses does not expose sensitive information or create a vector for security breaches when communicated outside trusted components.
  2. brave_wallet/browser/simulation_response_parser.cc:

    • Validate that parsing changes do not introduce any risk of interpreting the simulation response data incorrectly.
  3. General:

    • Confirm that the newly included timestamp for the Blowfish API version does not lead to any API incompatibilities or unexpected behaviors.
    • Verify that the addition of the userAccount to the transaction scan simulation payload meets all necessary privacy and security requirements.

These hotspot areas should be carefully reviewed and tested to ensure they meet the required security standards before merging the pull request.

Copy link
Collaborator

@supermassive supermassive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wallet core lgtm

Copy link
Collaborator

@supermassive supermassive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wallet core lgtm

@onyb onyb requested a review from a team as a code owner January 19, 2024 11:51
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jan 19, 2024
@brave-builds
Copy link
Collaborator

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

Copy link
Contributor

@josheleonard josheleonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend ++

Great work!

@brave-builds
Copy link
Collaborator

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

Copy link
Collaborator

@StephenHeaps StephenHeaps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS++

@onyb onyb merged commit 5a9ba3f into master Jan 19, 2024
19 checks passed
@onyb onyb deleted the f/wallet/blowfish-upgrade branch January 19, 2024 16:31
@github-actions github-actions bot added this to the 1.64.x - Nightly milestone Jan 19, 2024
"decimals": "6",
"supply": "1000000000",
"metaplexTokenStandard": "unknown",
"price": null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug on this relating to this field not being substituted. This was recently fixed in a separate PR [1]. A review of these tests should follow to make sure they cover both fail and success routes to make sure we are in general testing what we think to be testing. An [2] issue has been created for this review.

[1] #25643
[2] brave/brave-browser#41144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate away from deprecated Blowfish API
6 participants