Skip to content

Add type hints and clean up ABI code #253

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
Nov 24, 2021
Merged

Add type hints and clean up ABI code #253

merged 30 commits into from
Nov 24, 2021

Conversation

@algochoi
Copy link
Contributor

@algochoi algochoi commented Nov 24, 2021 •
edited
Loading

This PR aims to do some refactoring and clean up so the ABI code has type hints, change some relative imports to absolute imports and makes some minor documentation changes to reflect the newer design.

I also opted to use forward references in some places because we are still on 3.5 for now.

algochoi added 27 commits October 27, 2021 15:43
@algochoi
Start ABI JSON interaction
4cab7a8
@algochoi
Add static annoation
223b05e
@algochoi
Fix Method argument parsing
bad480d
@algochoi
Add ABI Typing to Method arguments
3b6f40c
@algochoi
Merge remote-tracking branch 'origin/develop' into algochoi/abi-inter…
7b732a6 
…action
@algochoi
[WIP] Add AtomicTransactionComposer build functions
7cf0608
@algochoi
[WIP] Sign and send atomic transaction groups
58cef31
@algochoi
Add unit tests for object parsing
43874b2
@algochoi
Clean up method calls
d3fe73d
@algochoi
Address PR comments on JSON objects
c7f4e14
@algochoi
Refactor ABI Type to ABIType so it can be exposed to outside world
72d172d
@algochoi
Add cucumber steps for ABI tests and update existing implementation s…
e96b740 
…o it can pass these tests
@algochoi
Refactor TransactionSigner to Abstract class and merge signatures whe…
e052fa7 
…n signing
@algochoi
Update testing to reflect json unit tests and composer tests
d55f9a6
@algochoi
Formatting and docstring fixes
a748f7b
@algochoi
Clean up imports
23f63bb
@algochoi
Fix unit test for appId
f21ec5d
@algochoi
Refactor some names and add txn as an arg type
d63b4d1
@algochoi
Partially address PR comments
5c9d28e
@algochoi
Add some additional checks for safety
9fa591a
@algochoi
Fix a step so we check for empty string instead of None
351099a
@algochoi
MInclude returns tests and check for a valid returns from a ABI call
155fc90
@algochoi
Addressing PR comments about type hints and returning self
2a3e6c9
@algochoi
Ensure group ids are zero when adding transactions
02a1bb0
@algochoi
Add type hints and clean up ABI types
008b09a
@algochoi
Merge branch 'develop' into algochoi/abi-type-hints
ab72fab
@algochoi
Change relative imports to absolute imports
d088d2c
@algochoi algochoi self-assigned this Nov 24, 2021
@algochoi algochoi requested a review from tzaffi November 24, 2021 16:39
@algochoi algochoi marked this pull request as ready for review November 24, 2021 16:39
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
jasonpaulos
jasonpaulos reviewed Nov 24, 2021
View reviewed changes
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.

This is a nice improvement, but please consider replacing instances of list with List[<the element type>] for further type support. I believe a plain list is equal to List[Any].

You can import List from the typing module.

tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi reviewed Nov 24, 2021
View reviewed changes
tzaffi
tzaffi approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

I am approving the PR as I don't have any beef with it being merged as is. None of my suggestions are mandatory.

@algochoi algochoi merged commit 0f521d0 into develop Nov 24, 2021
@algochoi algochoi deleted the algochoi/abi-type-hints branch November 24, 2021 21:11
This was referenced Nov 25, 2021
Closed
Merged
@algobarb algobarb mentioned this pull request Jan 14, 2022
Merged
aldur pushed a commit that referenced this pull request Feb 8, 2022
@algochoi @aldur
Add type hints and clean up ABI code (#253)
44d3b94 
* Add type hints and clean up ABI types

* Change relative imports to absolute imports

* Add type hints for ABI objects

* Refactor composer with type hints

* Address PR comments, change list type hints, and move type_from_string to ABIType
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jasonpaulos jasonpaulos jasonpaulos left review comments

@tzaffi tzaffi tzaffi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@algochoi algochoi

Labels

Team Scytale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@algochoi @tzaffi @jasonpaulos