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

onyb · 2024-05-17T12:16:46Z

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:

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:

See linked issue

components/brave_wallet/browser/eth_abi_decoder.cc

supermassive · 2024-05-24T10:03:43Z

components/brave_wallet/common/eth_abi_utils.h

+  explicit Type(TypeKind kind);
+  Type(TypeKind kind, size_t m);
+  ~Type();
+
+  // Delete copy constructor and copy assignment operator
+  Type(const Type&) = delete;
+  Type& operator=(const Type&) = delete;
+
+  // Define move constructor
+  Type(Type&& other) noexcept;
+};


these kinds of methods typically come first

supermassive · 2024-05-24T10:07:15Z

components/brave_wallet/common/eth_abi_utils.cc

+TypeBuilder Array(size_t m) {
+  return TypeBuilder(TypeKind::kArray, m);
+}
+
+Type Address() {
+  return Type{TypeKind::kAddress};
+}
+
+Type UintM(size_t m) {
+  return Type{TypeKind::kUintM, m};
+}
+
+Type Uint() {
+  return Type{TypeKind::kUintM, 256};
+}
+
+Type Bool() {
+  return Type{TypeKind::kBool};
+}
+
+Type Bytes() {
+  return Type{TypeKind::kBytes};
+}
+
+Type Bytes(size_t m) {
+  return Type{TypeKind::kBytes, m};
+}
+
+Type String() {
+  return Type{TypeKind::kString};
+}


let's CHECK(...) limits from spec where applicable
bytes<M>: binary type of M bytes, 0 < M <= 32.
uint<M>: unsigned integer type of M bits, 0 < M <= 256, M % 8 == 0

supermassive · 2024-05-24T10:08:34Z

components/brave_wallet/common/eth_abi_utils.h

+  TypeBuilder& setArrayType(Type array_type);
+  TypeBuilder& addTupleType(Type tuple_type);


SetArrayType
and
AddTupleType

supermassive · 2024-05-24T10:11:46Z

components/brave_wallet/browser/json_rpc_service.cc

@@ -1188,7 +1188,9 @@ void JsonRpcService::OnGetERC20TokenBalance(
    return;
  }

-  const auto& args = eth::DecodeEthCallResponse(*result, {"uint256"});
+  // (uint256)
+  auto type = eth_abi::Tuple().addTupleType(eth_abi::UintM(256)).build();


eth_abi::Uint() or I'd rather add Unit256() which is the least ambiguous

and same for two cases below

I find UintM(256) a better choice here compared to Uint() since the actual type is uint256, so there's less confusion.

Also is defining a Unit256() really necessary? The idea of having UintM(<size>) was to have less code. Btw I renamed UintM(<size>) to Uint(<size>).

supermassive · 2024-05-24T10:20:57Z

components/brave_wallet/browser/eth_data_parser.cc

+        std::vector<std::string>{"address",   // recipient
+                                 "uint256"},  // amount
+        std::vector<std::string>{decoded.value()[0].GetString(),
+                                 decoded.value()[1].GetString()});


Idea for future refactor: technically we can infer

std::vector<std::string>{"address", "uint256"},

from type

supermassive · 2024-05-24T10:35:34Z

components/brave_wallet/browser/eth_data_parser.cc

        std::vector<std::string>{fill_path,
                                 "",  // maker asset is ETH, amount is txn value
-                                 tx_args.at(1)});
+                                 decoded_calldata.value()[1].GetString()});


Why these tx_args are not

decoded_calldata.value()[0].GetString(), decoded_calldata.value()[1].GetString(), decoded_calldata.value()[2].GetString(),

?

The tx_args corresponds to the ETHSwap transaction type. The idea is to parse the calldata using the relevant ABI but eventually format it as per the tx_args expected by ETHSwap.

supermassive · 2024-05-24T10:36:33Z

components/brave_wallet/browser/eth_data_parser.cc

        std::vector<std::string>{"bytes",     // fill path,
                                 "uint256",   // maker amount
                                 "uint256"},  // taker amount
-        std::vector<std::string>{fill_path, tx_args.at(1), tx_args.at(2)});
+        std::vector<std::string>{fill_path,
+                                 decoded_calldata.value()[1].GetString(),
+                                 decoded_calldata.value()[2].GetString()});
  } else if (selector == kSellToUniswapSelector) {


why this doesn't match with type?

See #23730 (comment)

supermassive · 2024-05-24T10:41:50Z

components/brave_wallet/browser/eth_data_parser.cc

    return std::make_tuple(
        mojom::TransactionType::ETHSwap,
        std::vector<std::string>{"bytes",     // fill path,
                                 "uint256",   // maker amount
                                 "uint256"},  // taker amount
-        std::vector<std::string>{tx_args.at(0) + tx_args.at(1).substr(2),
-                                 tx_args.at(2), tx_args.at(3)});
+        std::vector<std::string>{


doesn't match with type at all

Btw this code has not been changed. We do not pass the exact parsed calldata in order to simply the frontend integration - they just need to know how to handle ETHSwap transactions.

supermassive · 2024-05-24T10:43:46Z

components/brave_wallet/browser/eth_abi_decoder.cc

+  // Unrecognized types are considered errors.
+  return std::nullopt;
+}


NOTREACHED_NORETURN

github-actions · 2024-06-03T16:56:33Z

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

Description

This PR updates the ABIDecode function in eth_abi_decoder.cc to use a new type system for representing Ethereum ABI types. The new type system is defined in eth_abi_utils.h and allows for more flexible and robust decoding of ABI-encoded data.

The main changes include:

Updating ABIDecode to take an eth_abi::Type object instead of a vector of type strings. This allows for decoding of complex nested types like tuples and arrays.
Refactoring the various Get*FromData helper functions to return a DecoderResult tuple containing the decoded value, remaining data, and number of bytes consumed. This allows for more efficient decoding of sequential values.
Adding support for decoding strings, fixed-size bytes, and nested arrays/tuples.
Updating tests and other code that calls ABIDecode to use the new type system.

Changes

Changes

eth_abi_decoder.cc:

Refactored ABIDecode to recursively decode values based on the provided eth_abi::Type
Updated Get*FromData helpers to return DecoderResult tuples
Added DecodeParam functions to decode individual values based on type
Added IsDynamicType helper to determine if a type is dynamic
Added GetTupleFromData and GetArrayFromData to decode nested types

eth_abi_decoder.h:

Updated ABIDecode function signature to take eth_abi::Type

eth_abi_decoder_unittest.cc:

Updated tests to use new eth_abi::Type builders when calling ABIDecode
Added tests for decoding arrays, tuples, strings, fixed bytes

eth_data_parser.cc:

Updated parsing code to use new eth_abi::Type when calling ABIDecode

eth_response_parser.cc:

Updated DecodeEthCallResponse to take eth_abi::Type

eth_response_parser_unittest.cc:

Updated tests to pass eth_abi::Type to DecodeEthCallResponse

json_rpc_service.cc:

Updated calls to eth::DecodeEthCallResponse to pass eth_abi::Type

eth_abi_utils.cc:

Added Type, TypeBuilder and type builder helper functions

eth_abi_utils.h:

Added TypeKind enum and Type struct to represent ABI types
Added TypeBuilder class and builder functions for each supported type

Overall, this is a significant refactor that improves the robustness and flexibility of the ABI decoding code. The new type system allows decoding complex nested values which was not possible before.

onyb self-assigned this May 17, 2024

onyb requested a review from a team as a code owner May 17, 2024 12:16

github-actions bot added feature/web3/wallet feature/web3/wallet/core puLL-Merge labels May 17, 2024

onyb mentioned this pull request May 17, 2024

feat(wallet): use LiFi provider for EVM swaps #23731

Open

24 tasks

supermassive reviewed May 21, 2024

View reviewed changes

components/brave_wallet/browser/eth_abi_decoder.cc Outdated Show resolved Hide resolved

onyb force-pushed the f/wallet/abi-decoder-improvements branch from e0d518e to 6b20104 Compare May 24, 2024 09:11

supermassive reviewed May 24, 2024

View reviewed changes

onyb added 6 commits June 3, 2024 18:55

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

0135aca

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

105f20c

review(supermassive): move method declarations in Type struct

53382d2

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

5a7269b

Remove unnecessary kBytesM field

2b119fd

review(supermassive): function rename

d577d75

onyb force-pushed the f/wallet/abi-decoder-improvements branch from 6b20104 to 1a9d2e5 Compare June 3, 2024 16:55

onyb added 2 commits June 3, 2024 19:46

review(supermassive): NOTREACHED_NORETURN

9636696

Rename UintM to Uint

a997420

onyb force-pushed the f/wallet/abi-decoder-improvements branch from 1a9d2e5 to a997420 Compare June 3, 2024 17:46

onyb requested review from supermassive and kdenhartog June 3, 2024 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

onyb commented May 17, 2024 •

edited

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

onyb Jun 3, 2024

supermassive May 24, 2024

onyb Jun 3, 2024 •

edited

github-actions bot commented Jun 3, 2024

Changes

		TypeBuilder& setArrayType(Type array_type);
		TypeBuilder& addTupleType(Type tuple_type);

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

Are you sure you want to change the base?

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

Conversation

onyb commented May 17, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onyb Jun 3, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Jun 3, 2024

Description

Changes

onyb commented May 17, 2024 •

edited

onyb Jun 3, 2024 •

edited