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

ABIv2 tuple encoding is not supported #1065

Closed
ghost opened this issue Sep 19, 2018 · 24 comments
Closed

ABIv2 tuple encoding is not supported #1065

ghost opened this issue Sep 19, 2018 · 24 comments

Comments

@ghost
Copy link

ghost commented Sep 19, 2018

  • Version: 4.7.1
  • Python: 3.6
  • OS: osx/linux/win

What was wrong?

ABIv2 tuple encoding is not supported, even though the currently used version of eth_abi does support these sort of encodings if invoked correctly. For this reason it is impossible to call the getOrderInfo() method (https://github.com/0xProject/0x-monorepo/blob/development/packages/migrations/artifacts/2.0.0/Exchange.json#L1370) from the 0x V2 Exchange contract. And many other methods as well.

The exception thrown is ValueError: No matching entries for 'tuple' in encoder registry.

@pipermerriam
Copy link
Member

Somewhat duplicate of #829

This is implemented in eth-abi but full support hasn't been implemented in web3.

@MatthiasLohr
Copy link
Contributor

Any news on this?

@fselmo
Copy link
Collaborator

fselmo commented Nov 29, 2021

@MatthiasLohr this open issue may have already been addressed by #1235 as mentioned in the closed duplicate issue linked above. @kclowes @marcgarreau any context I may be missing that warrants keeping this open?

@kclowes
Copy link
Collaborator

kclowes commented Nov 29, 2021

AFAICT this can be closed, but @MatthiasLohr do you have an example that isn't working?

@MatthiasLohr
Copy link
Contributor

MatthiasLohr commented Nov 30, 2021

I'm not sure. I have something which isn't working, but not completely sure who is the culprit.

I have the following nested Solidity structure (from the perun project) and a binary encoding method for it:

struct State {
    bytes32 channelID;
    uint64 version;
    Allocation outcome;
    bytes appData;
    bool isFinal;
}

struct Allocation {
    address[] assets;
    uint256[][] balances;
    SubAlloc[] locked;
}

struct SubAlloc {
    bytes32 ID;
    uint256[] balances;
    uint16[] indexMap;
}

function encodeState(State memory state) public pure returns (bytes memory)  {
  return abi.encode(state);
}

and its Python equivalent:

class ChannelState(NamedTuple):
    channel_id: bytes
    version: int
    outcome: Allocation
    app_data: bytes
    is_final: bool

    def abi_encode(self) -> bytes:
        return eth_abi.abi.encode_abi(
            [
                "(bytes32,uint64,(address[],uint256[][],(bytes32,uint256[],uint16[])[]),bytes,bool)"
            ],
            [tuple(self)],
        )

    def __iter__(self) -> Generator[Any, None, None]:
        yield self.channel_id
        yield self.version
        yield tuple(self.outcome)
        yield self.app_data
        yield self.is_final

class Allocation(NamedTuple):
    assets: List[ChecksumAddress] = []
    balances: List[List[int]] = [[]]
    locked: List[SubAllocation] = []

    def __iter__(self) -> Generator[Any, None, None]:
        yield self.assets
        yield self.balances
        yield [tuple(sub_alloc) for sub_alloc in self.locked]

class SubAllocation(NamedTuple):
    id: bytes
    balances: List[int] = []
    index_map: List[int] = []

Now, I can either invoke encodeState in the smart contract using web3 or I can do the encoding directly in Python using eth_abi.abi.encode_abi. I would expect to have the same results, but, for the following State object it doesn't work:

// non-working example
state = ChannelState(
    channel_id=generate_bytes(32),
    version=1,
    outcome=Allocation(),
    app_data=bytes(FileSaleState()),
    is_final=False,
)

test_web3_contract.functions.encodeState(tuple(state)).call()
// returns 0000000000000000000000000000000000000000000000000000000000000020abb78f11d032f958850ee75e8282a4e0aa6956f0b0553a103d091b7027385034000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000001a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000e000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

channel_state.abi_encode()
// returns 0000000000000000000000000000000000000000000000000000000000000020abb78f11d032f958850ee75e8282a4e0aa6956f0b0553a103d091b7027385034000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000001c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000e0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
// 32 times `0x00` more

Interestingly, if I do not have empty lists in the Allocation object, both encoding methods work:

// working example
state = ChannelState(
    channel_id=generate_bytes(32),
    version=999,
    outcome=Allocation(
        assets=[],
        balances=[[]],
        locked=[
            SubAllocation(id=generate_bytes(32), balances=[], index_map=[])
        ],
    ),
    app_data=b"abcdefg",
    is_final=False,
)

So, as I said before, not exactly sure why this is happening and who is the culprit (maybe it's me?). Searching for possible reasons for this behavior, I stumbled over this open issue. Maybe someone can help and show me the path to go.

@MatthiasLohr
Copy link
Contributor

Any ideas?

@kclowes
Copy link
Collaborator

kclowes commented Dec 8, 2021

Thanks for the example and the bump @MatthiasLohr. It looks like this may be a bug in eth-abi, but also looks like you have a workaround? We need to get a few things out the door, but this is on my list to look at.

@MatthiasLohr
Copy link
Contributor

Unfortunately, this workaround only works in a testbed for me, since I want to connect to an already existing smart contract, which does not have the encodingMethod public, it's internal only. So I really need a solution for this, but for now couldn't find any...

@kclowes
Copy link
Collaborator

kclowes commented Dec 8, 2021

Gotcha. And you can't use the "working example" you listed above passing the empty arrays?

// working example
state = ChannelState(
  channel_id=generate_bytes(32),
   version=999,
   outcome=Allocation(
       assets=[],
       balances=[[]],
       locked=[
           SubAllocation(id=generate_bytes(32), balances=[], index_map=[])
       ],
   ),
   app_data=b"abcdefg",
   is_final=False,
)

@MatthiasLohr
Copy link
Contributor

Unfortunately not, another part in the smart contracts checks for length of the error to be zero for specific cases (https://github.com/hyperledger-labs/perun-eth-contracts/blob/main/contracts/Adjudicator.sol#L256).

@fselmo
Copy link
Collaborator

fselmo commented Jan 21, 2022

@MatthiasLohr I'm seeing state and channel_state in your example that didn't work. Could that perhaps be the issue? Where is channel_state defined?

@fselmo
Copy link
Collaborator

fselmo commented Jan 21, 2022

I also wonder if your SubAllocation default value should be an empty SubAllocation rather than an empty list.

edit: oops, nope, I read that wrong

@fselmo
Copy link
Collaborator

fselmo commented Jan 21, 2022

This seems like a separate issue altogether (@MatthiasLohr), if you are still having issues here would you mind opening a new issue with a good description of what you are seeing and some code to duplicate?

I am going to close this as I believe it has been taken care of by #2211... but if that did not solve the issue please try to open this issue back up @reverendus and / or update here if you still have issues.

@fselmo fselmo closed this as completed Jan 21, 2022
@MatthiasLohr
Copy link
Contributor

No, it's not solved. As described above, there is a difference in python-based encoding and in-EVM encoding of empty arrays.

It's not up to me to decide about the application logic of the code excerpts showed above. I'm using a given Smart Contract framework (Perun) which has a semantical difference between [] and [0]. And even if I could change the application logic: It still remains a bug in the ABIv2 tuple encoding when it comes to nested elements with empty arrays. So, for me it's not even a different issue, since it's a bug in ABIv2 encoding, therefore this issue here is not fully and correctly implemented, therefore (in my opinion) it should not be closed.

My workaround: I'm not doing any encoding anymore using web3py - I created Solidity methods for all these encodings and call contract.functions.encodeWhatever(tuple(structure_to_be_encoded)).call(). Still: This is a workaroung for an existing bug. Not a solution.

@fselmo
Copy link
Collaborator

fselmo commented Jan 21, 2022

I understand. I'm closing this issue because what you are reporting is a separate issue altogether. You may create a new issue or I can but you have the best examples / code snippets for that. Would you like me to create it instead?

The original issue seems to be related to this specific error:

ValueError: No matching entries for 'tuple' in encoder registry

@fselmo
Copy link
Collaborator

fselmo commented Jan 21, 2022

@MatthiasLohr I did some poking around and I can definitely reproduce it on my end but I also found another scenario worth looking into. I think the best place to track this may actually be in eth-abi so I'm going to open an issue there. Thanks for bringing it to our attention I will link it in here so we can track it from this issue as well.

@MatthiasLohr
Copy link
Contributor

Thx for digging deeper into that!

@fselmo
Copy link
Collaborator

fselmo commented Jan 21, 2022

@MatthiasLohr, out of curiosity does the contract you are interacting with live on chain? I did my testing with eth-tester + py-evm so not really ideal... for the examples you ran into, were they with a deployed contract?

@MatthiasLohr
Copy link
Contributor

Not that I know of. I'm deploying it myself on ganache and locally running geth instance with a config similar to goerli. Anyway. the encoding methods are all private, so not callable from extern. That's the reason why I created a "helper contract" which just offers public pure methods for encoding as the workaround mentioned above. Contracts I'm talking about: https://github.com/hyperledger-labs/perun-eth-contracts/tree/main/contracts

@MatthiasLohr
Copy link
Contributor

@MatthiasLohr I did some poking around and I can definitely reproduce it on my end but I also found another scenario worth looking into. I think the best place to track this may actually be in eth-abi so I'm going to open an issue there. Thanks for bringing it to our attention I will link it in here so we can track it from this issue as well.

Did you already open the issue mentioned here?

@fselmo
Copy link
Collaborator

fselmo commented Jan 22, 2022

@MatthiasLohr, actually I did more testing and the discrepancies do seem equal in nature. They come about from zero padding and some other equivalent differences but they are the same instructions.

For a sanity check I added a decodeState(bytes memory _encoded) method to the contract and decoded both using that and compared that to decoding using eth-abi's decode_abi() and it all decodes successfully to the same object. I even played around with the kintsugi testnet (for fun mostly) and deployed a sample contract and sent transactions using the different data payloads and it all works out the same.

kintsugi transaction, eth-abi encoded payload for a similar example to your "non-working" example you have above:
https://explorer.kintsugi.themerge.dev/tx/0x9ec3e8d2cfbcf13d16e156d7cfe25fdcbdabfc15f1162886c9538af8b9253abf/internal-transactions

kintsugi transaction, contract encoded payload for the same example used in the above tx:
https://explorer.kintsugi.themerge.dev/tx/0x85dd5549d626298fa90420b0d68cf701b4c49fe06eefd957525d7ff71e59185f/internal-transactions

You can compare that the Raw Input (data) values for those transactions are not quite the same but they yield successful transactions with the same result. You can see the input decoded at the bottom. You can plug in your values in the Read Contract tab into the decodeState() method, with a 0x prefix, and see those results as well. They are the same from my tests.

Also, contract.functions.methodName(tuple(etc...)).transact() turned out exactly the same as using eth-abi in the way that you did. You just have to add the 4-byte function identifier to the beginning of the encoded object and 0x-prefix it for the data payload and fill in the rest of the transaction params.

Let me know if you do see some issues though... but I would imagine if this was not working it would have popped up sooner. I also tested with empty bytes for the app_data field in the struct and that also yields encoded data of differing lengths but the result is the same.

Happy to keep this conversation going if I overlooked something. I don't quite know if this is non-ideal in any situation (where differing instructions may be sent) but I'm going to continue to look into it... I would imagine errors would pop up in encoding if something did not quite match up though.

edit: To be clear, it's definitely a bug in the sense that it should be the same. But I'm glad at least the same instructions are being passed in as this would have led to other issues and I'm sure it would've popped up sooner.

@fselmo
Copy link
Collaborator

fselmo commented Jan 22, 2022

@MatthiasLohr I appreciate this observation quite a bit because if anything I think we may be able to improve on efficiency in the library. I believe that the eth-abi encoding may use a tad bit more gas and thus be more costly of a transaction. I think the issue that can be created is to look into making some improvements on the eth-abi library to see if we can bring it up to par with Solidity abi encoding. Thanks for raising this!

@MatthiasLohr
Copy link
Contributor

Thanks for investing so much time.

Actually, my problem is not about the actual encoding/decoding, but software doing hashing/digital signatures (as it is verified by the Perun smart contracts). So, what I actually want to achieve: Encode the input data and create a signature, which can be read from the contract. This can only work, if the input for the signature (or hash) is exactly the same on both sides, means, the encoding on both sides has to be exactly the same.

When reading the eth-abi documentation and function signatures, I would expect eth-abi encoding functions to behave exactly like the Solidity functions may do. Maybe encoding/decoding still works somehow, but creating signatures/hashes and verifying it can only work if the bytes representation is exactly the same. Since the whole blockchain thing heavily (or better: exclusively) relies on hashes and signatures, I would not classify this issue as "nice to be fixed" but "absolutely mandatory" that available off-chain/off-node/offline encoding functions return the exact same results as solidity would/will do when running on-chain.

@fselmo
Copy link
Collaborator

fselmo commented Jan 24, 2022

@MatthiasLohr for sure... this is definitely a bug. My first impression was that this was yielding incorrect instructions for the EVM and that's what I wanted to test above. I do think this will need to be corrected. Unfortunately I don't have the bandwidth for it at the moment but I may in a couple of weeks. PRs are always welcome in the eth-abi repo if you end up digging further and find where the issue lies. I have a hunch it's not solely in the zero-padding but if it is then that shouldn't be too difficult of a change.

Thanks again for raising awareness on this! I linked this issue over there as well. Glad you were able to at least find a work-around for now. Hopefully we can get it resolved soon 🙂

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

4 participants