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

[Ethereum]: Add support for abi.encodePacked() | Resolves #3679 #3862

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

theghostmac
Copy link

Description

This pull request adds support for abi.encodePacked() as described in issue #3679. The implementation follows the non-standard packed mode encoding, which concatenates types shorter than 32 bytes directly without padding or sign extension, encodes dynamic types in-place without the length field, and encodes array elements padded but in-place.

How to test

Reviewers can test the changes by running the included test cases that cover various scenarios for abi.encodePacked() encoding:

  1. cargo test encode_packed_tokens
  2. Specific test cases:
    • encode_packed_address
    • encode_packed_bytes
    • encode_packed_string
    • encode_packed_int
    • encode_packed_bool
    • encode_packed_uint
    • encode_packed_fixed_bytes
    • encode_packed_mixed
    • encode_packed_array

Ensure that all tests pass and verify the encoded outputs match the expected values.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • Create pull request as draft initially, unless it's 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.

Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Hi @theghostmac, thank you for opening the PR!
There seems to be protocol mismatches in Uint, Int encoding, could you please take a look?

rust/tw_evm/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 223 to 232
Token::Int { int, .. } => {
let buf = int.to_big_endian();
let trimmed_buf = trim_leading_zeros(buf.as_slice());
encoded.extend_from_slice(trimmed_buf);
},
Token::Uint { uint, .. } => {
let buf = uint.to_big_endian();
let trimmed_buf = trim_leading_zeros(buf.as_slice());
encoded.extend_from_slice(trimmed_buf);
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understood the specification correctly, integers are still padded according to the bits parameter.
So for example, uint16 (0x12) should be 0x0012, but this code will produce 0x12 instead

rust/tw_evm/src/abi/encode.rs Show resolved Hide resolved
}

#[test]
fn encode_packed_mixed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

done. does this look good enough?:

 #[test]
    fn encode_packed_custom_case() {
        // bytes32 stateRoots
        let state_roots = NonEmptyBytes::from_slice(
            "3a53dc4890241dbe03e486e785761577d1c369548f6b09aa38017828dcdf5c27"
                .decode_hex()
                .unwrap()
                .as_slice(),
        ).unwrap();

        // uint256[2] calldata signatures
        let signatures = Token::Array {
            arr: vec![
                Token::Uint {
                    uint: U256::from_str(
                        "3402053321874964899321528271743396700217057178612185975187363512030360053932"
                    ).unwrap(),
                    bits: UintBits::new(256).unwrap(),
                },
                Token::Uint {
                    uint: U256::from_str(
                        "1235124644010117237054094970590473241953434069965207718920579820322861537001"
                    ).unwrap(),
                    bits: UintBits::new(256).unwrap(),
                },
            ],
            kind: ParamType::Uint {
                bits: UintBits::new(256).unwrap(),
            },
        };

        // uint256 feeReceivers
        let fee_receivers = Token::Uint {
            uint: U256::zero(),
            bits: UintBits::new(256).unwrap(),
        };

        // bytes calldata txss
        let txss = Token::Bytes(
            "000000000000000100010000"
                .decode_hex()
                .unwrap(),
        );

        let tokens = vec![Token::FixedBytes(state_roots), signatures, fee_receivers, txss];
        let encoded = encode_packed_tokens(&tokens);

        let expected = "3a53dc4890241dbe03e486e785761577d1c369548f6b09aa38017828dcdf5c2707857e73108d077c5b7ef89540d6493f70d940f1763a9d34c9d98418a39d28ac02bb0e4743a7d0586711ee3dd6311256579ab7abcd53c9c76f040bfde4d6d6e90000000000000000000000000000000000000000000000000000000000000000000000000000000100010000"
            .decode_hex()
            .unwrap();

        assert_eq!(encoded, expected);
    }
    ```

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code can be cleaned up a bit if you move strings to a separate variable. For example,

let uint1 = "3402053321874964899321528271743396700217057178612185975187363512030360053932";

Token::Uint {
    uint: U256::from_str(uint1).unwrap(),
    bits: UintBits::new(256).unwrap(),
},

Anyway, does this test work?

Comment on lines 1232 to 1237
let expected = concat!(
"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", // -1
"42", // 0x42
"03", // 3
"48656c6c6f2c20776f726c6421" // "Hello, world!" in ASCII
).decode_hex().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test should be fixed accordingly

@@ -19,7 +19,7 @@ lazy_static! {

#[derive(Clone, PartialEq)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct I256(BaseU256);
pub struct I256(pub BaseU256);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding getter/setter functions instead

@@ -13,7 +13,7 @@ use tw_memory::Data;

#[derive(Copy, Clone, Debug, Default, PartialEq)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct U256(pub(crate) primitive_types::U256);
pub struct U256(pub primitive_types::U256);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add necessary getter/setter functions instead

Choose a reason for hiding this comment

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

Yo

@theghostmac
Copy link
Author

Hi, thanks @satoshiotomakan. I have addressed these.

Can you spare time to look at the corrections I made regarding Int and Uint before I open another PR?
Here:
master...theghostmac:wallet-core:add-abi-encode-packed-support.

@satoshiotomakan
Copy link
Collaborator

@theghostmac why not push changes to the same branch to avoid creating new PRs?
Otherwise we lose previous comments and change history

rust/tw_evm/src/abi/non_empty_array.rs Outdated Show resolved Hide resolved
rust/tw_evm/src/abi/non_empty_array.rs Outdated Show resolved Hide resolved
rust/tw_number/src/i256.rs Outdated Show resolved Hide resolved
rust/tw_number/src/u256.rs Outdated Show resolved Hide resolved
@theghostmac
Copy link
Author

Updated. Everything is in theghostmac:add-abi-encode-packed-support. I hope this matches your expectation.

The specific test case you sent from miguelmota, it passes also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants