-
Notifications
You must be signed in to change notification settings - Fork 18
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
base: master
Are you sure you want to change the base?
Conversation
Good year all ! ( cc @NagyZoltanPeter ) |
87fb8c1
to
b32602e
Compare
web3/eth_api_types.nim
Outdated
|
||
result.add quote do: | ||
type | ||
`identUint`* = StUint[`lastpow2`] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
conceptually, what we're trying to ensure is that web3 exposes exactly what the contract abi - there, Regarding stint and 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. |
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 usesUint32
(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#L141In 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#L33In 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 itsnim-web3
vendor.