web3j icon indicating copy to clipboard operation
web3j copied to clipboard

Updated TypeReference object to support nested types to support truly…

Open calmacfadden opened this issue 1 year ago • 3 comments

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 avatar Mar 26 '24 13:03 calmacfadden

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 :warning:
abi/src/main/java/org/web3j/abi/TypeReference.java 50.00% 4 Missing :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 02 '24 07:04 codecov[bot]

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

gtebrean avatar Apr 02 '24 12:04 gtebrean

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.

Antlion12 avatar Jun 24 '24 11:06 Antlion12

Can we push this in?

lukerQuant avatar Jul 01 '24 13:07 lukerQuant

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

gtebrean avatar Jul 05 '24 06:07 gtebrean

@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

lukerQuant avatar Jul 05 '24 08:07 lukerQuant

@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 avatar Jul 05 '24 18:07 Antlion12

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

gtebrean avatar Jul 09 '24 06:07 gtebrean

@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 avatar Jul 10 '24 07:07 Antlion12

I've created a fork of this in PR 2076. https://github.com/hyperledger/web3j/pull/2076

Antlion12 avatar Jul 10 '24 08:07 Antlion12

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

NickSneo avatar Aug 04 '24 22:08 NickSneo