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

ABI Type encoding support #238

Merged
merged 27 commits into from
Oct 8, 2021
Merged

ABI Type encoding support #238

merged 27 commits into from
Oct 8, 2021

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Sep 28, 2021

This PR adds ABI encoding support for the Python SDK:

Closes #228

@algochoi algochoi requested a review from ahangsu September 28, 2021 17:28
@algochoi algochoi marked this pull request as ready for review October 5, 2021 16:34
Copy link
Contributor Author

@algochoi algochoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current structure looks a bit redundant, e.g. you have to from algosdk.abi.type import UintType. I wonder if I should go back to putting this in algosdk/abi.py or a different approach in packaging this -- any thoughts are appreciated.

@jasonpaulos
Copy link
Contributor

The current structure looks a bit redundant, e.g. you have to from algosdk.abi.type import UintType. I wonder if I should go back to putting this in algosdk/abi.py or a different approach in packaging this -- any thoughts are appreciated.

I think it would be just fine if you move everything from abi/Type.py to abi.py, which remove the redundancy in code.

If the abi folder contains only a single file (besides __init__.py), I agree that it makes sense to get rid of the folder and just have an abi.py file.

But, what I think should actually happen is that abi/type.py should be broken up into many files (one for each class maybe?). Then abi/__init__.py can import all of the classes/functions which should be publicly available, meaning the end users will only need to do from algosdk.abi import UintType, ByteType, ...

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, I really like the use of an abstract base class for Type and the encode and decode methods on each type.

My comments are mainly about implementation and some minor interface improvements.

Additionally, could you add docstring comments to the encode and decode methods of each subclass? The reason being each subclass accepts and returns different types for these methods, so it would be good if we can document them all.

@algochoi algochoi requested a review from jasonpaulos October 7, 2021 18:47
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left a few nits which you can choose to accept or not. Pending tests, this looks good to merge

@algochoi algochoi merged commit e444328 into develop Oct 8, 2021
@algochoi algochoi deleted the abi-types branch October 28, 2021 15:20
aldur pushed a commit that referenced this pull request Feb 8, 2022
* Add initial support for ABI types

* Finish up abi types and values

* Add doc strings

* Refactor and finish encoding functions

* More cleanup

* Fix minor bugs and add number decoding

* Bump version to 1.8.0 and add changelog

* Add change to changelog

* Add decoding for arrays and dynamic types

* Add more unit tests

* Minor change from PR comment

* Another small PR comment change

* Split up files and remove circular imports

* Address PR comments

* Have arrays accept bytes and add docstrings

* Minor clean up

* Fix error messages for negative uint

* Remove unnecessary imports

* Refactor out abi_types since it is implicit by the class

* Tuples don't need static lengths...

* Address PR comments 1

* Fix head encoding placeholders

* Fix the tuple docstring

* Formatting fixes

Co-authored-by: Brice Rising <[email protected]>
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.

Add ABI encoding support
5 participants