esaulpaugh / headlong

High-performance Contract ABI and RLP for Ethereum
Apache License 2.0
79 stars 20 forks source link

Add `decodeArgs()` method for `Event` #33

Closed devictr closed 2 years ago

devictr commented 2 years ago

Hey @esaulpaugh,

Here's my implementation of the decode function for Events. I still need to add a few more tests for edge cases, but I wanted your opinion before doing so. In a few words:

This split between topics and data as input parameters is similar to what is done in ethereumj or to how things are presented in tools like etherscan.

Would that implementation work for you? Feel free to also let me know about any code style changes you would like me to make, in order to match the project's choices in that regard.

esaulpaugh commented 2 years ago

looks good so far. Other than more tests, all I can think to add right now is private static final ArrayType<ByteType, byte[]> BYTES_32 = TypeFactory.create("bytes32");

esaulpaugh commented 2 years ago

I would probably also eliminate decodedTopicsTuple and just use decodedTopics[topicIndex++]

devictr commented 2 years ago

@esaulpaugh Fixed your comments and added more tests

esaulpaugh commented 2 years ago

Okay, thanks. I will look at it.

esaulpaugh commented 2 years ago

I'm thinking that if it's going to be doing decoding, Event should derive indexed and non-indexed TupleTypes only once, when constructed, and reuse them. Right? Unless there's a compelling reason not to, I think Event could just keep references to three TupleTypes: inputs, indexed, and nonIndexed.

And non-anonymous Events would calculate topics[0] once during construction and use it to verify the topics arrays it's decoding with Arrays.equals? new Keccak(256).digest(Strings.decode(getCanonicalSignature(), Strings.ASCII))

What do you think?

devictr commented 2 years ago

That sounds great! Adding those things now

devictr commented 2 years ago

@esaulpaugh Here you go! I opted to throw a RuntimeException when the signature hashes don't match for non-anonymous events, but I'm thinking we should probably create our own exception for that. What do you think? Maybe there's another standard exception we could use

devictr commented 2 years ago

I also refactored my code a bit and extracted some parts into more methods

esaulpaugh commented 2 years ago

I usually just throw IllegalArgumentException for anything attributable to the caller.

devictr commented 2 years ago

Fixed! If this looks good to you and you decide to merge it, would you mind cutting a release for it? That way I can merge some internal PR I have open as well 🙂 I'd also be happy to contribute more. If you open issues for things you need help with, I'd love to take a look when I have time

esaulpaugh commented 2 years ago

LGTM. I will be releasing version 6.0.0 with this included soon.

Thanks. After the release I will go through the abi-spec some more and see if there is anything else I'm missing.

devictr commented 2 years ago

Great! Thank you