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.

Sign up for GitHub

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

Jump to bottom

feat(wallet): extend ABI decoder to support dynamic and nested types #23730

Merged
merged 13 commits into from
Jun 11, 2024
Merged

feat(wallet): extend ABI decoder to support dynamic and nested types #23730

merged 13 commits into from
Jun 11, 2024

Conversation

onyb
Copy link
Member

@onyb onyb commented May 17, 2024 •
edited

Resolves brave/brave-browser#38402

Improved ABIDecode to handle arbitrary calldata with complex types. The result is now encoded into a base::Value to be generic. Also added a new parser for LiFi transactions, which utilises these the decoder improvements.

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:

Test Plan:

See linked issue

@onyb onyb self-assigned this May 17, 2024
@onyb onyb requested a review from a team as a code owner May 17, 2024 12:16
@onyb onyb mentioned this pull request May 17, 2024
Merged
24 tasks
@onyb onyb force-pushed the f/wallet/abi-decoder-improvements branch from e0d518e to 6b20104 Compare May 24, 2024 09:11
@onyb onyb force-pushed the f/wallet/abi-decoder-improvements branch 2 times, most recently from 1a9d2e5 to a997420 Compare June 3, 2024 17:46
@onyb onyb force-pushed the f/wallet/abi-decoder-improvements branch from a997420 to 8417c6a Compare June 10, 2024 21:12
@onyb onyb requested a review from supermassive June 10, 2024 21:17
@supermassive
Copy link
Collaborator

wallet core lgtm

@github-actions GitHub Actions
Copy link
Contributor

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

Here's my review of the PR:

Description

This PR makes significant changes to the ABIDecode function in eth_abi_decoder.cc to support decoding more Ethereum ABI types, including tuples, arrays, strings, and bytes. It introduces a new eth_abi::Type class to represent the structure of the ABI types being decoded. The motivation seems to be extending the transaction data parsing capabilities to handle more complex method calls.

Changes

Changes

  • eth_abi_decoder.cc

    • Refactored ABIDecode to use the new eth_abi::Type class to specify the expected ABI type structure, rather than a vector of strings.
    • Added support for decoding tuples, arrays, strings, bytes, and more integer types.
    • Return a base::Value object containing the decoded values rather than a tuple of params and args vectors.
    • Many new helper functions to recursively decode the different types.

  • eth_abi_decoder.h

    • Updated ABIDecode function signature.
    • Added include for eth_abi_utils.h

  • eth_abi_decoder_unittest.cc

    • Updated existing tests to use the new ABIDecode interface.
    • Added new test cases for tuples, arrays, strings.

  • eth_data_parser.cc and eth_data_parser_unittest.cc

    • Updated parsing logic to use the new ABIDecode interface
    • Added parsing support for some new transaction types like LiFi swap orders.

  • eth_response_parser.cc and eth_response_parser.h

    • Updated DecodeEthCallResponse to take an eth_abi::Type param instead of a vector of strings.

  • json_rpc_service.cc

    • Updated calls to DecodeEthCallResponse

  • eth_abi_utils.cc and eth_abi_utils.h

    • Added new Type, TypeBuilder classes and factory functions to represent ABI type structures.

Security Hotspots

None found. The changes are focused on enhancing the ABI decoding capabilities and don't seem to introduce any new attack surface or vulnerabilities.

Overall this looks like a solid set of changes to improve the Ethereum ABI decoding support in the Brave wallet. The new eth_abi::Type abstraction makes the expected ABI structure clearer when invoking ABIDecode. The unit tests have been properly updated. No major issues or security concerns found in this PR.

Let me know if you have any other questions!

kdenhartog
kdenhartog approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Overall, this refactor LGTM. I think it would be useful to split this ABIDecode logic out onto the separate utility process, but that will likely require a larger consideration into how that should be done to address this on iOS. I don't think this is something that we need to address in the PR, but I'll add an issue for it so we can address when we get a bit more time (lower priority IMO).

components/brave_wallet/browser/eth_abi_decoder.cc
}

offset += 32;
std::optional<base::Value::List> ABIDecode(const eth_abi::Type& type,
Copy link
Member

Choose a reason for hiding this comment

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

nit: it would be nice to move this into BraveWalletUtilsService as a defense in depth measure

@onyb onyb merged commit 0e12319 into master Jun 11, 2024
19 checks passed
@onyb onyb deleted the f/wallet/abi-decoder-improvements branch June 11, 2024 13:10
@github-actions github-actions bot added this to the 1.69.x - Nightly milestone Jun 11, 2024
@kdenhartog kdenhartog mentioned this pull request Jun 11, 2024
Open
jagadeshjai pushed a commit to jagadeshjai/brave-core that referenced this pull request Jun 12, 2024
@onyb @jagadeshjai
feat(wallet): extend ABI decoder to support dynamic and nested types (b…
ded5179 
…rave#23730)

* feat(wallet): extend ABI decoder to support dynamic and nested types

* review(supermassive): use builder pattern to construct ABI types

* review(supermassive): move method declarations in Type struct

* review(supermassive): add CHECK limits for uint<m> and bytes<m>

* Remove unnecessary kBytesM field

* review(supermassive): function rename

* review(supermassive): NOTREACHED_NORETURN

* Rename UintM to Uint

* review(supermassive): convert DecoderResult to struct

* review(supermassive): add CHECK_EQ

* review(supermassive): avoid casting to double

* review(supermassive): reduce allocations using ByteView struct

* review(supermassive): use base::span for ByteView
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supermassive supermassive supermassive approved these changes

@kdenhartog kdenhartog kdenhartog approved these changes

Assignees

@onyb onyb

Labels
feature/web3/wallet/core feature/web3/wallet puLL-Merge
Projects
None yet
Milestone
1.69.x - Nightly
3 participants
@onyb @supermassive @kdenhartog