Add ABI encoding support #2807
Merged
Add ABI encoding support #2807
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jannotti
reviewed
Aug 27, 2021
There was a problem hiding this comment.
This is really, really big, but I don't know what actually changed in the goal interface. Could you write up what has been added?
I also would expect to see some very, very concise tests that take a string, and show the proper decoding / encoding. One after another, with edge cases described in comments.
jannotti
reviewed
Aug 27, 2021
jannotti
reviewed
Aug 27, 2021
jannotti
reviewed
Aug 27, 2021
jannotti
reviewed
Aug 27, 2021
jannotti
reviewed
Aug 27, 2021
|
Got it, ok.
…On Fri, Aug 27, 2021 at 12:17 PM Hang Su ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In cmd/goal/abi/abi.go
<#2807 (comment)>:
> + bigIntBytes := bigIntValue.Bytes()
+ buffer := make([]byte, v.valueType.typeSize/8-uint16(len(bigIntBytes)))
+ buffer = append(buffer, bigIntBytes...)
+ return buffer, nil
FillBytes is introduced in 1.15 (ref
<https://pkg.go.dev/math/big#Int.FillBytes>), I have to manually fill
bytes to work with old golang that go-algorand and go-algorand-sdk
support...
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2807 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADL7T6SWJQ62RRJAYQXYO3T663C5ANCNFSM5C2A5QUA>
.
|
|
Yes, I agree, our messages were out of sync.
…On Fri, Aug 27, 2021 at 12:25 PM Hang Su ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In cmd/goal/abi/abi.go
<#2807 (comment)>:
> + bigIntBytes := bigIntValue.Bytes()
+ buffer := make([]byte, v.valueType.typeSize/8-uint16(len(bigIntBytes)))
+ buffer = append(buffer, bigIntBytes...)
+ return buffer, nil
Yes, you are correct about using GetUint for size checking, but the point
I am trying to make is, this project supports Golang 1.14, where FillBytes
is not supported, so this is just an ugly work-around that I do not like
too.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2807 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADL7T5P244DANNPIHZZHRTT664ANANCNFSM5C2A5QUA>
.
|
jasonpaulos
suggested changes
Aug 27, 2021
Codecov Report
@@ Coverage Diff @@
## master #2807 +/- ##
==========================================
+ Coverage 47.08% 47.37% +0.28%
==========================================
Files 349 352 +3
Lines 56401 57044 +643
==========================================
+ Hits 26558 27024 +466
- Misses 26865 26982 +117
- Partials 2978 3038 +60
Continue to review full report at Codecov.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
algochoi
reviewed
Sep 21, 2021
algochoi
reviewed
Sep 21, 2021
jannotti
reviewed
Sep 22, 2021
jasonpaulos
reviewed
Sep 22, 2021
jasonpaulos
reviewed
Sep 22, 2021
jasonpaulos
previously approved these changes
Sep 22, 2021
jannotti
reviewed
Sep 25, 2021
| case Byte: | ||
| return "byte" | ||
| case Ufixed: | ||
| return "ufixed" + strconv.Itoa(int(t.bitSize)) + "x" + strconv.Itoa(int(t.precision)) |
There was a problem hiding this comment.
No need to change, but I fmt.Sprintf("ufixed%dx%d", t.bitSize, t.precision) might be easier on the eyes.
jannotti
reviewed
Sep 25, 2021
Comment on lines
+119
to
+125
| staticArrayRegexp, err = regexp.Compile(`^([a-z\d\[\](),]+)\[([1-9][\d]*)]$`) | ||
| if err != nil { | ||
| panic(err.Error()) | ||
| } | ||
| ufixedRegexp, err = regexp.Compile(`^ufixed([1-9][\d]*)x([1-9][\d]*)$`) | ||
| if err != nil { | ||
| panic(err.Error()) |
There was a problem hiding this comment.
Simplify with https://pkg.go.dev/regexp#MustCompile ? I suppose that could even make it doable outside init() at global level.
There was a problem hiding this comment.
I removed the init function, put the regexp compile to global level.
jannotti
approved these changes
Sep 25, 2021
2 tasks
ahangsu
added a commit
to algorand/java-algorand-sdk
that referenced
this pull request
Oct 11, 2021
Corresponding implementation to algorand/go-algorand#2807 - serialization/deserialization of ABI types (and the corresponding testing) - Encoding/Decoding of JAVA values with ABI type schemes (with tests)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR provides a prototype implementation that closes #2348:
Test Plan
For both ABI Type serialization/deserialization and ABI Value Encoding/Decoding: