Adds ability to take an ABI to handle `EntryFunction` JSON arguments correctly #4190

gupnik · 2024-12-31T05:34:57Z

Description

Fixes #3899

This PR adds the ability to accept an ABI in the SigningInput so that we are able to parse the arguments correctly.

How to test

Run Rust Tests

Types of changes

Add abi in tw_proto::Aptos::Proto::SigningInput.

Checklist

Create pull request as draft initially, unless its complete.
Add tests to cover changes as needed.
Update documentation as needed.
If there is a related Issue, mention it in the description.

If you're adding a new blockchain

I have read the guidelines for adding a new blockchain.

satoshiotomakan

@gupnik thanks for the PR! Please consider the following notes

rust/chains/tw_aptos/Cargo.toml

rust/chains/tw_aptos/src/aptos_move_types.rs

rust/chains/tw_aptos/src/transaction_payload.rs

rust/chains/tw_aptos/tests/signer.rs

src/proto/Aptos.proto

github-actions · 2025-01-02T09:55:44Z

Binary size comparison

➡️ aarch64-apple-ios:

- 12.49 MB
+ 12.52 MB 	 +34 KB

➡️ aarch64-apple-ios-sim:

- 12.49 MB
+ 12.53 MB 	 +34 KB

➡️ aarch64-linux-android:

- 16.12 MB
+ 16.17 MB 	 +52 KB

➡️ armv7-linux-androideabi:

- 13.75 MB
+ 13.79 MB 	 +40 KB

➡️ wasm32-unknown-emscripten:

- 11.43 MB
+ 11.47 MB 	 +41 KB

10gic · 2025-01-03T08:08:56Z

Hi @gupnik ,

Thanks for the PR! Aptos has its own ABI definition. For example: Abi example.

Can we use the office ABI format instead? I'm concerned that the simple ABI introduced in this PR may not cover more complex cases.

gupnik · 2025-01-03T10:28:28Z

Hi @gupnik ,

Thanks for the PR! Aptos has its own ABI definition. For example: Abi example.

Can we use the office ABI format instead? I'm concerned that the simple ABI introduced in this PR may not cover more complex cases.

Hi @10gic, thanks for the comment.

As the user needs to provide an individual payload anyways to blind sign, I am not sure if we need the complete ABI parser. The current PR just expects the types of the arguments and ensures that they are parsed to TransactionArguments correctly.

So, the caller should simply be able to retrieve the ABI json via the API: https://aptos.dev/en/build/apis/fullnode-rest-api-reference#tag/accounts/GET/accounts/{address}/modules and call the function with the respective types.

10gic · 2025-01-03T12:57:29Z

Hi @gupnik ,
Thanks for the PR! Aptos has its own ABI definition. For example: Abi example.
Can we use the office ABI format instead? I'm concerned that the simple ABI introduced in this PR may not cover more complex cases.

Hi @10gic, thanks for the comment.

As the user needs to provide an individual payload anyways to blind sign, I am not sure if we need the complete ABI parser. The current PR just expects the types of the arguments and ensures that they are parsed to TransactionArguments correctly.

So, the caller should simply be able to retrieve the ABI json via the API: https://aptos.dev/en/build/apis/fullnode-rest-api-reference#tag/accounts/GET/accounts/{address}/modules and call the function with the respective types.

We don't need to pass the entire ABI JSON file; it's sufficient to only pass the params field.

The params field can have complex cases. For example:

            "params": [
                "vector<vector<u8>>",
                "vector<vector<u8>>",
                "vector<u64>",
                "0x1::coin::Coin<0x1::aptos_coin::AptosCoin>"
            ]

            "params": [
                "&signer",
                "vector<vector<u8>>",
                "vector<vector<u8>>",
                "vector<u64>"
            ]

            "params": [
                "0x7e783b349d3e89cf5931af376ebeadbfab855b3fa239b7ada8f5a92fbea6b387::price_identifier::PriceIdentifier",
                "u64"
            ],

These examples are taken from: https://aptoscan.com/module/0x7e783b349d3e89cf5931af376ebeadbfab855b3fa239b7ada8f5a92fbea6b387/pyth

Can you add more unit tests to improve coverage?

gupnik · 2025-01-06T05:54:20Z

Hi @gupnik ,
Thanks for the PR! Aptos has its own ABI definition. For example: Abi example.
Can we use the office ABI format instead? I'm concerned that the simple ABI introduced in this PR may not cover more complex cases.

Hi @10gic, thanks for the comment.
As the user needs to provide an individual payload anyways to blind sign, I am not sure if we need the complete ABI parser. The current PR just expects the types of the arguments and ensures that they are parsed to TransactionArguments correctly.
So, the caller should simply be able to retrieve the ABI json via the API: https://aptos.dev/en/build/apis/fullnode-rest-api-reference#tag/accounts/GET/accounts/{address}/modules and call the function with the respective types.

We don't need to pass the entire ABI JSON file; it's sufficient to only pass the params field.

The params field can have complex cases. For example:
            "params": [
                "vector<vector<u8>>",
                "vector<vector<u8>>",
                "vector<u64>",
                "0x1::coin::Coin<0x1::aptos_coin::AptosCoin>"
            ]

            "params": [
                "&signer",
                "vector<vector<u8>>",
                "vector<vector<u8>>",
                "vector<u64>"
            ]

            "params": [
                "0x7e783b349d3e89cf5931af376ebeadbfab855b3fa239b7ada8f5a92fbea6b387::price_identifier::PriceIdentifier",
                "u64"
            ],
These examples are taken from: https://aptoscan.com/module/0x7e783b349d3e89cf5931af376ebeadbfab855b3fa239b7ada8f5a92fbea6b387/pyth

Can you add more unit tests to improve coverage?

Thanks @10gic for raising this. I went through aptos-core and it seems that the encoding can be implemented via the following steps:

Accept the abi and decode each parameter into a MoveType
Obtain the TypeTag as shows in tests here
Convert into a MoveTypeLayout as shown here
- This internally goes to TypeBuilder here
- Accordingly, this states that an input cannot be a signer while others are converted appropriately
Create a MoveValue as shown here which can be appropriately serialized.

I will try to implement a similar flow and the respective unit tests for serialisation but please let me know if you would want me try to broadcast any specific transactions.

CC: @satoshiotomakan

satoshiotomakan · 2025-01-06T08:52:04Z

@gupnik looks good. From my point of view, unit tests with only value parsing would be enough, if we borrow them from aptos-core repository. Can you please leave comments with the links to the original unit tests when finish?

gupnik · 2025-01-07T07:10:16Z

@gupnik looks good. From my point of view, unit tests with only value parsing would be enough, if we borrow them from aptos-core repository. Can you please leave comments with the links to the original unit tests when finish?

@satoshiotomakan I have added the implementation as discussed. Also, here are the tests that I have replicated as well.

To note: aptos-core uses a module resolver internally to obtain the struct types. In order to be able to accept that as an API, I have added the support for generics.

For example, in the following case, aptos-core is able to retrieve the types of 0x1::guid::ID and accordingly parse the input.

assert_value_conversion(
        &converter,
        "0x1::guid::ID",
        json!({"addr": "0x1", "creation_num": "1"}),
        VmMoveValue::Struct(MoveStruct::Runtime(vec![
            VmMoveValue::U64(1),
            VmMoveValue::Address(address),
        ])),
    );

In our case, we instead are able to accept it as follows:

assert_value_conversion(
      "0x1::guid::ID<u64, address>",
      ["1", "0x1"],
      MoveValue::Struct(MoveStruct::Runtime(vec![
          MoveValue::U64(1),
          MoveValue::Address(address),
      ])),
  );

satoshiotomakan

Thanks for the completing the implementation!
I have several minor and a few important questions/change requests. Could you please take a look?

rust/chains/tw_aptos/src/aptos_move_types.rs

satoshiotomakan · 2025-01-07T15:39:41Z

rust/chains/tw_aptos/src/transaction_payload.rs

+            } else {
+                return Err(EncodingError::InvalidInput);
+            }


aptos-core allows the struct tag to be a non-utf8 string, but any other.

aptos-core allows the struct tag to be a non-utf8 string, but any other.

Yeah, aptos-core handles strings and structs with named fields, whereas we handle strings and structs with unnamed fields due to the reasoning mentioned here.

rust/chains/tw_aptos/src/transaction_payload.rs

satoshiotomakan · 2025-01-07T15:53:19Z

rust/chains/tw_aptos/src/transaction_payload.rs

+        );
+
+        assert_value_conversion(
+            "0x1::guid::ID<u64, address>", // As we do not have access to the module resolver, the types of the struct should be provided as params


Does https://aptos.dev/en/build/apis/fullnode-rest-api-reference#tag/accounts/GET/accounts/{address}/modules RPC return this type argument with the type arguments like 0x1::guid::ID<u64, address>, or it returns 0x1::guid::ID?
If it doesn't have type arguments, then the WalletCore users won't be able to provide the full ABI required by WalletCore

Does https://aptos.dev/en/build/apis/fullnode-rest-api-reference#tag/accounts/GET/accounts/{address}/modules RPC return this type argument with the type arguments like 0x1::guid::ID<u64, address>, or it returns 0x1::guid::ID? If it doesn't have type arguments, then the WalletCore users won't be able to provide the full ABI required by WalletCore

They should be able to use https://aptos.dev/en/build/apis/fullnode-rest-api-reference#tag/accounts/GET/accounts/{address}/resource/{resource_type} to retrieve the types

gupnik · 2025-01-07T16:22:30Z

Hi @satoshiotomakan, thanks for the review. I will go through each of them in my day tomorrow, but to clarify each line/conversion has only been added because it is needed. I will try to share how & where its used tomorrow.

satoshiotomakan · 2025-01-07T16:27:25Z

Hi @gupnik,

Ok no problem, if they're all used, no need to post comments, thanks! I'll resolve corresponding questions

each line/conversion has only been added because it is needed. I will try to share how & where its used tomorrow.

gupnik added 2 commits December 30, 2024 17:42

Adds ability to provide abi in signing input

5ca47be

Minor

63097f2

satoshiotomakan requested changes Dec 31, 2024

View reviewed changes

gupnik added 2 commits January 2, 2025 14:19

Addresses review comments

a2c369b

Adds broadcasting info

b53749b

gupnik marked this pull request as ready for review January 2, 2025 09:16

gupnik requested a review from Milerius as a code owner January 2, 2025 09:16

Minor

b724c6e

gupnik added 3 commits January 3, 2025 11:55

Adds kotlin, swift and C++ unit tests

467f9ff

Makes clippy happy

ae2627c

FMT

14d0c97

Adds successful transaction in unit tests

5533663

gupnik added 3 commits January 7, 2025 12:32

Implements generic parsing

15b5c0d

FMT

0eac919

Adds missing

0f8ffee

satoshiotomakan requested changes Jan 7, 2025

View reviewed changes

gupnik and others added 2 commits January 8, 2025 10:24

Adds reference links

6fdd360

Merge branch 'master' into gupnik/aptos-abi

f9dc6c0

satoshiotomakan merged commit 1ce5282 into master Jan 8, 2025
14 checks passed

satoshiotomakan deleted the gupnik/aptos-abi branch January 8, 2025 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds ability to take an ABI to handle `EntryFunction` JSON arguments correctly #4190

Adds ability to take an ABI to handle `EntryFunction` JSON arguments correctly #4190

gupnik commented Dec 31, 2024

satoshiotomakan left a comment

github-actions bot commented Jan 2, 2025 •

edited

Loading

10gic commented Jan 3, 2025

gupnik commented Jan 3, 2025 •

edited

Loading

10gic commented Jan 3, 2025

gupnik commented Jan 6, 2025 •

edited

Loading

satoshiotomakan commented Jan 6, 2025

gupnik commented Jan 7, 2025

satoshiotomakan left a comment

satoshiotomakan Jan 7, 2025

gupnik Jan 8, 2025

satoshiotomakan Jan 7, 2025

gupnik Jan 7, 2025 •

edited

Loading

gupnik commented Jan 7, 2025

satoshiotomakan commented Jan 7, 2025

Adds ability to take an ABI to handle EntryFunction JSON arguments correctly #4190

Adds ability to take an ABI to handle EntryFunction JSON arguments correctly #4190

Conversation

gupnik commented Dec 31, 2024

Description

How to test

Types of changes

Checklist

satoshiotomakan left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 2, 2025 • edited Loading

Binary size comparison

10gic commented Jan 3, 2025

gupnik commented Jan 3, 2025 • edited Loading

10gic commented Jan 3, 2025

gupnik commented Jan 6, 2025 • edited Loading

satoshiotomakan commented Jan 6, 2025

gupnik commented Jan 7, 2025

satoshiotomakan left a comment

Choose a reason for hiding this comment

satoshiotomakan Jan 7, 2025

Choose a reason for hiding this comment

gupnik Jan 8, 2025

Choose a reason for hiding this comment

satoshiotomakan Jan 7, 2025

Choose a reason for hiding this comment

gupnik Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

gupnik commented Jan 7, 2025

satoshiotomakan commented Jan 7, 2025

Adds ability to take an ABI to handle `EntryFunction` JSON arguments correctly #4190

Adds ability to take an ABI to handle `EntryFunction` JSON arguments correctly #4190

github-actions bot commented Jan 2, 2025 •

edited

Loading

gupnik commented Jan 3, 2025 •

edited

Loading

gupnik commented Jan 6, 2025 •

edited

Loading

gupnik Jan 7, 2025 •

edited

Loading