-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add ABI encoding support #255
Conversation
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.
This is a good start, and I really like the approach of having a base Type
and Value
class and subclasses for every ABI type. It seems like it will be easier to add new ABI types in the future with this approach.
However, I would suggesting changing the subclass names from <abi-type>T
and <abi-type>V
to Type<abi-type>
and Value<abi-type>
, e.g. TypeUint
and ValueUint
.
src/main/java/com/algorand/algosdk/util/abi/values/ValueUfixed.java
Outdated
Show resolved
Hide resolved
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.
This is looking very clean, just left some minor initial comments.
src/main/java/com/algorand/algosdk/util/abi/types/TypeArrayStatic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/util/abi/types/TypeUfixed.java
Outdated
Show resolved
Hide resolved
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.
The more I think about it, the more the Python/Javascript approach of having no Value
classes makes more sense to me. With the current implementation, this is the code needed to encode a value of type (uint64,byte[])
:
Value intValue = new ValueUint(76L);
Value[] byteValues = new Value[]{
new ValueByte((byte)1),
new ValueByte((byte)2),
new ValueByte((byte)3),
};
Value byteArrayValue = new ValueArrayDynamic(byteValues, new TypeByte());
Value value = new ValueTuple(new Value[]{ intValue, byteArrayValue });
byte[] encoded = value.encode();
Comparatively, if there were no Value
classes and the Type
class had methods public byte[] encode(Object value)
and public Object decode(byte[])
, this is what it could look like:
Type type = Type.fromString("(uint64,byte[])");
long intValue = 76;
byte[] byteValues = new byte[]{ 1, 2, 3 };
List<Object> tuple = List.of(intValue, byteValues);
type.encode(tuple);
The second seems significantly shorter, easier to write, and safer, since the type string would validate the Java values being encoded against the schema. Apologies for brining this up rather late, but what do you think?
(I also left some smaller comments)
src/main/java/com/algorand/algosdk/util/abi/types/TypeAddress.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/util/abi/types/TypeArrayDynamic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/util/abi/types/TypeArrayStatic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/util/abi/types/TypeArrayDynamic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/algorand/algosdk/util/abi/values/ValueAddress.java
Outdated
Show resolved
Hide resolved
Removing It seems to me that a rough structure of code change is shaped. Do we need to further reformat the code in go-algorand? |
Great! I also agree it would be beneficial to do a similar thing for the Go SDK and take advantage of But there is one issue that will make the Go implementation more annoying than Java or any of the other languages. That is that you cannot view a slice of concrete types as a "general" slice, e.g. For example, in Java we can use the wildcard type Object objectToEncode = ...;
if (objectToEncode instanceof List<?>) {
List<?> asList = (List<?>) objectToEncode;
for (Object item : asList) {
// encode each child
}
} But in Go, if we want to support a tuple encoding function that can accept any slice type, we would need to do: var input interface{} = ...
switch value := input.(type) {
case []interface{}:
// handle a slice of interface{} to support tuples containing different types
case []uint8:
// handle a uint8 array to support tuples/arrays of all uint8s
case []uint16:
// ... repeat for every supported type
} I still think it's worth it because the resulting API will be much easier to use, but it might require some long switch statements 😅 |
src/main/java/com/algorand/algosdk/util/abi/TypeArrayDynamic.java
Outdated
Show resolved
Hide resolved
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.
It looks a lot cleaner without the Value
classes!
Most of my comments are about Java type conversions. Could you also add some simple unit tests to check that these types are properly accepted? E.g. List<Integer>
, Integer[]
, and int[]
should all be valid input types to Type.fromString("uint64[]").encode
src/main/java/com/algorand/algosdk/util/abi/TypeArrayDynamic.java
Outdated
Show resolved
Hide resolved
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.
Looking good, just one final comment
Additionally, could you add docstrings for isDynamic
, byteLen
, encode
, and decode
on the Type
class?
This reverts commit e3c9614.
This reverts commit 1debe4e.
(Resolved conflicts) commit ffc8254 Merge: 6e7cbec 7ae8485 Author: egieseke <[email protected]> Date: Thu Dec 30 13:58:19 2021 -0500 Merge branch 'release/1.11.0-beta-2' into develop commit 7ae8485 Author: egieseke <[email protected]> Date: Thu Dec 30 09:59:13 2021 -0500 bump to 1.11.0-beta-2 commit 6e7cbec Author: Hang Su <[email protected]> Date: Tue Dec 28 16:38:14 2021 -0500 Support Foreign objects as ABI arguments (#277) * update method/arg/returns/interface/contract definition * ugh no lambda in java7 * update method parsing, bug fix * update * update name checker and test cases * update testcase for method * update testing for contract and interface * update background types * update methods in atomic transaction composer * update methods in atomic transaction composer * update methods in atomic transaction composer, execute and add method call to go * need to update exed * update return value format * put method call option separate * update method, allowing argument to have tx type in string * update method list to map * update clone * update exec * update execute method * update params * yea this should break, add abijson test * update cucumber test for abijson * update cucumber test script * update * yea this one also fail cuz you need atomic transaction composer test in cucumber * there is a bug for transaction argument test, going to fix that * update minor fix to get through test * start working on integrate test for abi * resolve issues partly * working on integration test * make payment trans test in other class * half way through cucumber test * does not go through exec, dunno why * seeing some light ahead, compact the signedTxns with correct way * why returned log is empty? * minor renaming, and removing forced regex method name check * minor changes to rename stuffs and export stuffs * minor * minor * modify transactionSigner to an interface * minor update, update parse error in exec res * minor, return value cannot find abi res, do not throw exception * update sign method to sign interface hash code * update method arg ret with abitype, resolve build error * remove ABIValue and methodArgument * add method call polish * remove redundant lines * dirty fix for sdk not handling log return * update final step check, should pass... * change according to generator * updated naming * update naming * use generator again * minor * minor * resolve issues * start supporting foreign objects * update method test cases * supporting foreign array in atomic transaction composer * update compact foreign array, update array limit check, update method call allowing foreign array * update sender appid/addr to first apat/apas * minor * should be fixed * minor * first commit * update tuple wrapping mechanism, wrap from > 14 to > 15 * update contract appID to networks, should fail * minor * minor renaming * update cucumber test * clean up foreign object array support * minor * minor * minor on group size and group ID * revert test branch, get lost code back * update changes against cucumber test * update SDK testing implementation * resolve part of the review * remove some numerical constants by defining static final variables * minor * minor * optimize index to wait in execute * remove foreign object number constrain * per review * minor for ref type set * minor for contract networks non null * remove wildcard imports * minor * remove optimization on add method call commit 33652fe Author: Hang Su <[email protected]> Date: Wed Dec 8 16:39:02 2021 -0500 Upgrade `jackson` packages to resolve PRs on vulnerability update jackson-(core/databind/annotations) to resolve PR #279 and #229 commit b89681a Merge: 1c0e4f4 d398d0f Author: egieseke <[email protected]> Date: Tue Dec 7 16:45:05 2021 -0500 Merge branch 'release/1.11.0-beta-1' into develop commit d398d0f Author: egieseke <[email protected]> Date: Tue Dec 7 15:36:13 2021 -0500 Update Changelog. commit 2855578 Author: egieseke <[email protected]> Date: Tue Dec 7 14:53:02 2021 -0500 Removed PRs for 267, 269, and 270 from Changelog. commit 8aa70c8 Author: egieseke <[email protected]> Date: Tue Dec 7 11:34:00 2021 -0500 bump to 1.11.0-beta-1 commit 1c0e4f4 Author: Hang Su <[email protected]> Date: Fri Dec 3 13:49:24 2021 -0500 ABI Interaction Support for JAVA SDK (#268) This PR enables invoking on-chain contract methods using off-chain applications, as defined by ARC-0004. Design doc here: https://gist.github.com/jasonpaulos/a810abe7e86d43840d14445718565a9a commit 53b5583 Author: Hang Su <[email protected]> Date: Mon Nov 29 13:09:11 2021 -0500 Bug fix for `logs` on `PendingTransactionResponse` (#275) - update generator for new v2 model in java-sdk - add minor test for PendingTransactionResponse for retrieving logs commit 3086ecd Author: Hang Su <[email protected]> Date: Wed Nov 10 18:28:25 2021 -0500 Add WaitForConfirmation function (#274) Export WaitForConfirmation from test to SDK commit cf7248f Author: Will Winder <[email protected]> Date: Thu Oct 14 13:10:35 2021 -0400 Better error message on encoding exception. (#258) commit b830635 Author: Jason Paulos <[email protected]> Date: Tue Oct 12 13:29:04 2021 -0700 Revert "Revert "Fix ABI source code position for ABI feature (#260)"" This reverts commit 2bd5d77. commit 605bf4e Author: Jason Paulos <[email protected]> Date: Tue Oct 12 13:28:48 2021 -0700 Revert "Revert "Add ABI encoding support (#255)"" This reverts commit 1debe4e. commit b89da61 Merge: 04b497b cda8a83 Author: Jason Paulos <[email protected]> Date: Tue Oct 12 13:28:25 2021 -0700 Merge branch 'master' into develop commit 04b497b Merge: c488a10 c8f60e9 Author: Brice Rising <[email protected]> Date: Tue Oct 12 15:26:53 2021 -0400 Merge branch 'release/1.10.0' into develop
Corresponding implementation to algorand/go-algorand#2807
Closes #244