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 object to support nested types to support truly… #2023

Closed

Conversation

calmacfadden
Copy link
Contributor

What does this PR do?

This PR adds support for decoding dynamic structs without the need for a class extending DynamicStructs to be added at compile time.

I have extended the TypeReference class to have nested innerType's which means you can represent a Struct using TypeReference.

Where should the reviewer start?

  • Look at the TypeReference.java changes to see the innerTypes which have been added.
  • Check the decodeDynamicStruct function in TypeDecoder.java. I have kept the current decoding method intact but renamed it decodeDynamicStructElementsWithCtor and created a new function called decodeDynamicStructElementsWithInnerTypeRefs which is a duplicate of the original function but it has been modified to support the nested type refereces.
  • Check the unit test testDynamicStructFix() in the DefaultFunctionEncoderTest.java which demonstrates how it is expected to be used.

Why is it needed?

There needs to be a way to decode dynamic structs without having to create a corresponding class in the project. This allows for the ability to interact with any contract dynamically based on user inputs.

Checklist

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

@calmacfadden calmacfadden marked this pull request as draft March 26, 2024 13:39
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 86.46617% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 69.31%. Comparing base (ccfef8b) to head (cff3845).
Report is 57 commits behind head on master.

Files Patch % Lines
abi/src/main/java/org/web3j/abi/TypeDecoder.java 88.70% 12 Missing and 2 partials ⚠️
abi/src/main/java/org/web3j/abi/TypeReference.java 50.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2023      +/-   ##
============================================
+ Coverage     69.22%   69.31%   +0.08%     
- Complexity     3117     3143      +26     
============================================
  Files           493      493              
  Lines         13090    13230     +140     
  Branches       1692     1710      +18     
============================================
+ Hits           9062     9170     +108     
- Misses         3537     3569      +32     
  Partials        491      491              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gtebrean
Copy link
Contributor

gtebrean commented Apr 2, 2024

Great work! I've bee trough the changes and this new approach with extension of the TypeReference is easy to follow.
Also I can see that the checks are passing.
There might be some small comments in terms of code structure but we keep that for later.

Will be helpful if you can add some more test for testing of decoding and encoding of the new struct type reference. Ideally to cover all the existing test cases for DynamicStruct

@Antlion12
Copy link
Contributor

This seems like the missing piece for decoding event data without having to delcare in-Java classes apriori. Would be great if something like this was incorporated.

@lukerQuant
Copy link

Can we push this in?

@gtebrean
Copy link
Contributor

gtebrean commented Jul 5, 2024

@calmacfadden do you plan to continue the development on this?

@lukerQuant
Copy link

@calmacfadden do you plan to continue the development on this?

He has completed his contract at Quant now and therefore this github account is no longer active. I'm unsure if he will pick up this work privately, I would assume not.

So can we either merge it as is or find someone else to complete it? We currently don't have any other resource for this

@Antlion12
Copy link
Contributor

Antlion12 commented Jul 5, 2024

@calmacfadden do you plan to continue the development on this?

He has completed his contract at Quant now and therefore this github account is no longer active. I'm unsure if he will pick up this work privately, I would assume not.

So can we either merge it as is or find someone else to complete it? We currently don't have any other resource for this

I ended up implementing a version of this PR in my own work for decoding structs. Should I tack it on to this PR? Or start a new PR with the modifications?

@gtebrean
Copy link
Contributor

gtebrean commented Jul 9, 2024

@calmacfadden do you plan to continue the development on this?

He has completed his contract at Quant now and therefore this github account is no longer active. I'm unsure if he will pick up this work privately, I would assume not.
So can we either merge it as is or find someone else to complete it? We currently don't have any other resource for this

I ended up implementing a version of this PR in my own work for decoding structs. Should I tack it on to this PR? Or start a new PR with the modifications?

@Antlion12 please go for it and if it is possible to track it in this PR will be great.

@Antlion12
Copy link
Contributor

@Antlion12 please go for it and if it is possible to track it in this PR will be great.

I tried to push my changes to calmacfadden:TrulyDynamicStructs, but I don't have permission to push to that branch. I may have to fork this again.

@Antlion12
Copy link
Contributor

I've created a fork of this in PR 2076. #2076

@NickSneo
Copy link
Contributor

NickSneo commented Aug 4, 2024

Closing this outdated PR, as new PR created by @Antlion12 - #2076

@NickSneo NickSneo closed this Aug 4, 2024
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.

5 participants