-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Updated TypeReference and TypeDecoder to support decoding of dynamic structs and dynamic struct arrays without a priori Class definitions. #2076
Conversation
b8c953f
to
4f8689c
Compare
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.
please add 3 more test cases except these to ensure the functionality and split some functionalities in different methods for an easier tracking of the flow
abi/src/test/java/org/web3j/abi/DefaultFunctionEncoderTest.java
Outdated
Show resolved
Hide resolved
1f7e018
to
20a0947
Compare
Sounds good. I'll chip away at these. Question about the "add 3 more test cases" request: the test |
Sorry I forgot to mention, add dynamic struct of static struct, dynamic struct with type reference of dynamic arrays and static struct of dynamic struct in order to make sure that different use cases were covered |
@Antlion12 I tried to add the required changes on this PR in order to can include it in the next release but on the first required test case I notice that the DynamicStruct of StaticStruct is not supported, you can notice in the added test that the decode is failing. |
237e229
to
9a36a21
Compare
… dynamic stucts. Signed-off-by: Antlion12 <antlion@treasure.lol>
… unit test). Change visibility of TypeReference's innerTypes and getSubTypeReference() to protected/public to allow for overriding in anonymous classes. Redo naming of some innerType-specific functions in TypeDecoder to minimize code diffs. Signed-off-by: Antlion12 <antlion@treasure.lol>
Signed-off-by: Antlion12 <antlion@treasure.lol>
Signed-off-by: Antlion12 <antlion@treasure.lol>
…amicStructElementsFromInnerTypes(). Signed-off-by: Antlion12 <antlion@treasure.lol>
Signed-off-by: gtebrean <99179176+gtebrean@users.noreply.github.com> Signed-off-by: Antlion12 <antlion@treasure.lol>
Signed-off-by: gtebrean <99179176+gtebrean@users.noreply.github.com> Signed-off-by: Antlion12 <antlion@treasure.lol>
…rrayOfStaticStruct() and testBuildDynamicStructOfStaticStruct() unit tests that use the innerTypes version. Signed-off-by: Antlion12 <antlion@treasure.lol>
e4037da
to
d35b8fa
Compare
To fix the unit test, I have added support for building StaticStruct with innerTypes. I have also added unit tests of DynamicStruct of StaticStruct (built with inner types), as well as DynamicArray of StaticStruct (built with inner types). |
I agree with the changes, from what I see only the unit test StaticStruct of DynamicStruct is missing. Once you commit that I will merge this PR. |
Does a StaticStruct of DynamicStruct need to be tested if it's not considered valid ABI? I was looking at this part of the ABI encoding spec and saw
So if there was an outer struct that contained a DynamicStruct inside of it, then that outer struct can only be Dynamic (it couldn't be Static). Likewise, a StaticArray of a DynamicStruct is not valid ABI, since That's my understanding of it, at least. |
Indeed, I omitted this, my bad. |
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.
Please update the CHANGELOG.md according to the model and solve my last comment in order to can merge this PR
Signed-off-by: Antlion12 <antlion@treasure.lol>
Signed-off-by: Antlion12 <antlion@treasure.lol>
Signed-off-by: Antlion12 <antlion@treasure.lol>
I have renamed |
What does this PR do?
This PR builds off of the PR originally created by calmacfadden at #2023.
Where should the reviewer start?
Look in TypeDecoder.java, with attention to the decodeDynamicParameterFromStructWithTypeReference() and
decodeDynamicStructElementsFromInnerTypes(). Note that the Ctor() version of functions from PR 2023 have been
renamed to omit the Ctor so as to match the name in the main branch and reduce code diffs.
The unit test testArrayOfDynamicStruct() provides an example of decoding an array of dynamic structs (uint256,bool,string)[]
based on a transaction from the Treasure Ruby testnet.
I've taken some stylistic liberty by renaming TypeReference.innerTypeReferences() to TypeReferences.innerTypes().
I've changed the scope of some fields in TypeReference from private to protected/public to allow for creating the
anonymous DynamicArray class seen in testArrayOfDynamicStruct().
Why is it needed?
As with PR 2023, it would be useful to decode dynamic structs without having to explicitly define a corresponding class.
Checklist