-
Notifications
You must be signed in to change notification settings - Fork 18
feat: abi encoder #216
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
feat: abi encoder #216
Conversation
7a165a6 to
5580e75
Compare
|
I did a benchmark that shows the current implementation outperforms |
oh - that's interesting - if I were two guess, there's an extra allocation somewhere, probably made by faststreams .. |
emizzle
left a comment
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.
Really nice effort, Arnaud! 👏 I'll have to come back around to reviewing the encoding side of things in another review round as it's quite a lot to review in one sitting.
| var data: seq[seq[byte]] = @[] | ||
| var offset = totalSerializedFields(T) * abiSlotSize | ||
|
|
||
| value.enumInstanceSerializedFields(_, fieldValue): |
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.
since we know the size of the non-dynamic part of the encoding, we can use https://github.com/status-im/nim-faststreams/blob/a9c6b884b0971c03e2251094ac88d1ecbc2c20bb/faststreams/outputs.nim#L380 to reserve a write area for it and write the data directly to the stream, avoiding the need for var data
See https://github.com/status-im/nim-faststreams/blob/master/tests/test_outputs.nim#L474 for an example
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.
Okay done in my last commit 763a399
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.
nice! I'm curious if it helped the benchmark
|
lgtm and certainly a big improvement over the status quo!
Fine with merging and opening issues for the above, or fixing them in this PR directly |
I prefer to fix them in the PR directly |
|
Okay I fixed everything except the |
| @@ -0,0 +1,437 @@ | |||
| import | |||
| std/typetraits, | |||
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.
odd indent
web3/decoding.nim
Outdated
| decoder.input.advance(offsets[i].int - pos) | ||
| result[i] = decoder.decode(T) | ||
|
|
||
| return result |
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.
if result is used, we should not use return - alternatively, use a separate variable and expression return
| resultObj = decoder.decode(T) | ||
|
|
||
| decoder.finish() | ||
| result = resultObj |
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.
| result = resultObj | |
| resultObj |
broadly, we encourage expression return: https://status-im.github.io/nim-style-guide/language.result.html
|
LGTM! Let's get status-im/nimbus-eth2#7376 bumped to these latest changes with the delayed writes and merge as soon as that is green. |
Okay I am gonna to merge it, nimbus PR is green (except Lint because this PR is not on master yet) https://github.com/status-im/nimbus-eth2/actions/runs/17289414384/job/49073028914. |
* Updates to support the new json_serialization streaming * New ABI encoder/decoder (status-im/nim-web3#216)
Add ABI encoder / decoder from https://github.com/codex-storage/nim-contract-abi.
Fixes #213, #215 and #45