-
Notifications
You must be signed in to change notification settings - Fork 195
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
ABI Support for JS library #454
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.
I'm not sure why the build is failing. If you rebase/merge with develop, that might help
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.
I have the following file organization suggestions, what do you think?
- Move the functions
bigIntToBytes
andbytesToBigInt
to a new filesrc/encoding/bigint.ts
, since they are useful in general Right now all ABI code is in a folder by itself. Since we can't split up the file easily without introducing circular imports, I think moving the file toEDIT: actually having thesrc/encoding/abi.ts
would make more sense.abi
folder makes sense, since ABI JSON description handling code can live there too.
Also, some exporting suggestions:
- I just realized if we export the code as is, users would do
import { Type } from 'algosdk'
, which is not great becauseType
is pretty general. Probably renaming toABIType
makes more sense. src/main.ts
needs to be updated to actually export the new class. If you adopt the above changes,export { ABIType } from './encoding/abi';
should do it
(somehow half my review got submitted early, that was not intentional)
Weirdly it looks like the latest commit is failing on travis because of new cucumber tests from algorand/algorand-sdk-testing#145, but that shouldn't be happening because they're hidden behind the edit: will be fixed by algorand/algorand-sdk-testing#147 |
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.
Looks good, except there are still some bigint literals 😄
After fixing, this looks good to merge to me!
* added abi files * Temporarily disable linter for require() warning * Update tests for ABI types * Minor import change * Remove separate encode file * Add basic encoding tests * Enforce Bigint args in uint/ufixed types * Fix linter errors * Fix more linting errors * Address PR comments * Add decoding functions and tests * Fix chrome test errors by disabling crypto * Address PR comments and try to see if we can resolve Travis errors * Resolve circular dependencies in tests * Change exports, naming and file structure * Change bigint literals to BigInt() * Remove more bigint literals Co-authored-by: shiqi.zheng@algorand.com <shiqi.zheng@algorand.com>
This PR adds ABI type encoding support for the JS SDK library.
Closes #425