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

disasm: Names for Tuple Components are not set #148

Open
yohamta opened this issue Nov 5, 2024 · 6 comments
Open

disasm: Names for Tuple Components are not set #148

yohamta opened this issue Nov 5, 2024 · 6 comments

Comments

@yohamta
Copy link
Contributor

yohamta commented Nov 5, 2024

Problem

When inferring ABIs from bytecode, WhatsABI currently omits names for tuple components that are likely derived from Solidity structs. While the ABI specification technically allows unnamed tuple components, Solidity always generates names for struct members when creating the ABI. This discrepancy causes compatibility issues with libraries like go-ethereum that expect these names to be present when working with structs (See https://github.com/ethereum/go-ethereum/blob/v1.14.11/accounts/abi/type.go#L181).

Example contract:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

contract TestC {

    struct MyTuple {
        uint256 num;
        string str;
        bool flag;
    }

    function getTuple() external pure returns (MyTuple memory) {
        return MyTuple(123, "hello", true);
    }

    function setTuple(MyTuple calldata inputTuple) external pure returns (uint256, string memory, bool) {
        return (inputTuple.num, inputTuple.str, inputTuple.flag);
    }
}

ABI Generated by Solidity Compiler:

{
  "contracts": {
    "test.sol:TestC": {
      "abi": [
        {
          "inputs": [],
          "name": "getTuple",
          "outputs": [
            {
              "components": [
                {
                  "internalType": "uint256",
                  "name": "num",
                  "type": "uint256"
                },
                {
                  "internalType": "string",
                  "name": "str",
                  "type": "string"
                },
                {
                  "internalType": "bool",
                  "name": "flag",
                  "type": "bool"
                }
              ],
              "internalType": "struct TestC.MyTuple",
              "name": "",
              "type": "tuple"
            }
          ],
          "stateMutability": "pure",
          "type": "function"
        },
        {
          "inputs": [
            {
              "components": [
                {
                  "internalType": "uint256",
                  "name": "num",
                  "type": "uint256"
                },
                {
                  "internalType": "string",
                  "name": "str",
                  "type": "string"
                },
                {
                  "internalType": "bool",
                  "name": "flag",
                  "type": "bool"
                }
              ],
              "internalType": "struct TestC.MyTuple",
              "name": "inputTuple",
              "type": "tuple"
            }
          ],
          "name": "setTuple",
          "outputs": [
            {
              "internalType": "uint256",
              "name": "",
              "type": "uint256"
            },
            {
              "internalType": "string",
              "name": "",
              "type": "string"
            },
            {
              "internalType": "bool",
              "name": "",
              "type": "bool"
            }
          ],
          "stateMutability": "pure",
          "type": "function"
        }
      ]
    }
  },
  "version": "0.8.25+commit.b61c2a91.Darwin.appleclang"
}

Proposed solution

Fill in the component names with _n when they're empty.

Fill in the component names with field${n} when they're empty.

Example:

{
   "type": "function",
   "selector": "0x95d376d7",
   "payable": false,
   "stateMutability": "payable",
   "inputs": [
     {
       "type": "tuple",
       "name": "",
       "components": [
         { "type": "uint32", "name": "" },
         { "type": "bytes", "name": "" },
         { "type": "bytes32", "name": "" },
         { "type": "uint64", "name": "" },
         { "type": "address", "name": "" }
       ]
     },
     { "type": "bytes", "name": "" }
   ],
   "sig": "assignJob((uint32,bytes,bytes32,uint64,address),bytes)",
   "name": "assignJob",
   "constant": false
}

To be:

{
  "type": "function",
  "selector": "0x95d376d7",
  "payable": false,
  "stateMutability": "payable",
  "inputs": [
    {
      "type": "tuple",
      "name": "",
      "components": [
        { "type": "uint32", "name": "field0" },
        { "type": "bytes", "name": "field1" },
        { "type": "bytes32", "name": "field2" },
        { "type": "uint64", "name": "field3" },
        { "type": "address", "name": "field4" }
      ]
    },
    { "type": "bytes", "name": "" }
  ],
  "sig": "assignJob((uint32,bytes,bytes32,uint64,address),bytes)",
  "name": "assignJob",
  "constant": false
}

EDIT: Adjusted the placeholder format that should be supported by geth.

@shazow
Copy link
Owner

shazow commented Nov 5, 2024

HMMM! That is annoying behaviour on go-ethereum's part!

I'm generally hesitant to add made up results that WhatsABI did not actually extract. It would make it harder to distinguish if the ABI actually has a field named _0 vs the disassembler just assigning it that name.

I guess some options:

  1. Add a helper for augmenting any ABI with missing names into something go-ethereum-friendly.
  2. Add an optional field to autoload to use helper from (1). Not sure if this should be the default? Technically a breaking change, I guess.
  3. Just do it by default and abstract away from the user.

What do you think?

@yohamta
Copy link
Contributor Author

yohamta commented Nov 6, 2024

@shazow I understand your concern; that makes a lot of sense. From a user’s perspective, I think having a Geth-friendly ABI generated by default might be preferable.

@shazow
Copy link
Owner

shazow commented Nov 6, 2024

@yohamta I'm hesitant to make it default but I'll consider it. Would you be interested in contributing this? Could start with a standalone helper like abiFillEmptyNames that takes an ABI and fills in any empty names, then we can figure out where to insert it for autoload?

@yohamta
Copy link
Contributor Author

yohamta commented Nov 7, 2024

That sounds good to me! I'll open a draft PR later so we can discuss further.

@shazow
Copy link
Owner

shazow commented Nov 8, 2024

@yohamta Do you have a deployed contract/whatsabi code to reproduce this issue by the way?

@yohamta
Copy link
Contributor Author

yohamta commented Nov 10, 2024

@shazow Yes, here is the contract that reproduces the issue:
https://mantlescan.xyz/address/0x806feB7478b61bf068aA12C9413C6Ad7DD12232a

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

No branches or pull requests

2 participants