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

Adapt encoding to ethereum #186

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

Conversation

Ivansete-status
Copy link

@Ivansete-status Ivansete-status commented Jan 2, 2025

When sending a raw transaction to an Ethereum smart contract, we need to make sure all the types are 32-byte encoded, regardless their original type definition. For example, in nwaku we are dealing with a smart contract that uses Uint32 (4 bytes) but these types need to be sent 32-byte encoded.

As an example, when performing register operation, we have the following parameter in the RLN smart contract: https://github.com/waku-org/waku-rlnv2-contract/blob/a576a8949ca20e310f2fbb4ec0bd05a57ac3045f/src/WakuRlnV2.sol#L141

In turn, we have the following equivalent definition in nwaku: https://github.com/waku-org/nwaku/blob/09f05fefe2614d640277f96ae96ab4f2f12c8031/waku/waku_rln_relay/group_manager/on_chain/group_manager.nim#L33


In this PR we are introducing additional types that will be used by nwaku only.
I've validated that nimbus-eth2 still compiles all ( make ) with this commit in its nim-web3 vendor.

@Ivansete-status
Copy link
Author

Good year all !
@arnetheduck - I think this is not the cleanest solution and maybe we need to adapt our nwaku codebase to make sure a fixed 32-byte is always used. wdyt?

( cc @NagyZoltanPeter )


result.add quote do:
type
`identUint`* = StUint[`lastpow2`]
Copy link
Contributor

Choose a reason for hiding this comment

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

This in general will create some compatible-looking types in Nim which aren't actually compatible Solidity types, e.g., int40 and int48 are not the same in Solidity but they would end up being effectively the same in this mapping. Is that intention? Does it matter?

But this wouldn't hold between int64 and int72: those would map to 64 and 128 respectively, and would not be the same type. So this also becomes a question of, is that ok for your use case? It creates sort of distinctions in the Nim-side integer types which are different than the Solidity ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this always creates StInt/StUint types, even for lastpow2 <= 64. Maybe not optimal.

Copy link
Member

Choose a reason for hiding this comment

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

would end up being effectively the same in this mapping

This might be an actual problem - ie we should give a compile-time error rather than being incompatible at runtime for "unsupported" integers (or support them properly)

@arnetheduck
Copy link
Member

conceptually, what we're trying to ensure is that web3 exposes exactly what the contract abi - there, uintN is allowed for any N where N mod 8 and N <= 256 holds - an ideal leap forward here would capture the entire spectrum of allowed values - a minimal leap forward would just ensure compatibility with these values.

Regarding stint and <sizeof(int)*8 values, I think it's fine to allow that usage since stint[48] is how we would "express" a 48-bit integer in an contract.

One thing missing from this PR is a test case that encodes and decodes the newly specified types - ie there should be a test with some constants (perhaps generated using some web3 tooling) against which the encoders/decoders are tested, also for the new lengths.

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