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

Updated TypeReference and TypeDecoder to support decoding of dynamic structs and dynamic struct arrays without a priori Class definitions. #2076

Merged
merged 13 commits into from
Aug 8, 2024

Conversation

Antlion12
Copy link
Contributor

@Antlion12 Antlion12 commented Jul 10, 2024

  • Updated TypeReference object to support nested types to support truly dynamic stucts.
  • Support decoding of DynamicArray of DynamicStruct (with corresponding 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.

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

  • I've read the contribution guidelines.
  • I've added tests (if applicable).
  • I've added a changelog entry if necessary.

Copy link
Contributor

@gtebrean gtebrean left a 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/main/java/org/web3j/abi/TypeDecoder.java Outdated Show resolved Hide resolved
abi/src/main/java/org/web3j/abi/TypeDecoder.java Outdated Show resolved Hide resolved
abi/src/main/java/org/web3j/abi/TypeDecoder.java Outdated Show resolved Hide resolved
@Antlion12
Copy link
Contributor Author

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

Sounds good. I'll chip away at these.

Question about the "add 3 more test cases" request: the test testDynamicStructFix() covers dynamic struct inside a dynamic struct. The testArrayOfDynamicStruct() covers an array of dynamic struct. What other permutations are missing?

@gtebrean
Copy link
Contributor

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

Sounds good. I'll chip away at these.

Question about the "add 3 more test cases" request: the test testDynamicStructFix() covers dynamic struct inside a dynamic struct. The testArrayOfDynamicStruct() covers an array of dynamic struct. What other permutations are missing?

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

@gtebrean
Copy link
Contributor

gtebrean commented Aug 4, 2024

@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.
Meantime you find the time to get back to it I will also try to find a fix when I have the time.

@gtebrean gtebrean force-pushed the TrulyDynamicStructs branch 2 times, most recently from 237e229 to 9a36a21 Compare August 4, 2024 19:22
calmacfadden and others added 10 commits August 4, 2024 23:48
… 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 <[email protected]>
Signed-off-by: Antlion12 <[email protected]>
…amicStructElementsFromInnerTypes().

Signed-off-by: Antlion12 <[email protected]>
Signed-off-by: gtebrean <[email protected]>
Signed-off-by: Antlion12 <[email protected]>
Signed-off-by: gtebrean <[email protected]>
Signed-off-by: Antlion12 <[email protected]>
Signed-off-by: gtebrean <[email protected]>
Signed-off-by: Antlion12 <[email protected]>
Signed-off-by: gtebrean <[email protected]>
Signed-off-by: Antlion12 <[email protected]>
…rrayOfStaticStruct() and testBuildDynamicStructOfStaticStruct() unit tests that use the innerTypes version.

Signed-off-by: Antlion12 <[email protected]>
@Antlion12
Copy link
Contributor Author

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).

@gtebrean
Copy link
Contributor

gtebrean commented Aug 6, 2024

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.

@Antlion12
Copy link
Contributor Author

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

Definition: The following types are called “dynamic”:
...

  • T[k] for any dynamic T and any k >= 0
  • (T1,...,Tk) if Ti is dynamic for some 1 <= i <= k

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 T being dynamic impleis T[k] is dynamic.

That's my understanding of it, at least.

@gtebrean
Copy link
Contributor

gtebrean commented Aug 7, 2024

ABI encoding spec

Indeed, I omitted this, my bad.

Copy link
Contributor

@gtebrean gtebrean left a 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

abi/src/main/java/org/web3j/abi/TypeDecoder.java Outdated Show resolved Hide resolved
Signed-off-by: Antlion12 <[email protected]>
@Antlion12
Copy link
Contributor Author

I have renamed numNestedFields() to countNestedFields(), and i have updated the CHANGELOG.md to refer to this PR using the description "Add struct support in java without the need of having a corresponding Java class " provided from the 2024--2025 roadmap (https://wiki.hyperledger.org/display/WEB3J/Roadmap+2024+-+2025).

@gtebrean gtebrean merged commit bb9dc1c into hyperledger:main Aug 8, 2024
5 checks passed
@Antlion12 Antlion12 deleted the TrulyDynamicStructs branch August 8, 2024 20:42
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