-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
reorganized contract #2567
reorganized contract #2567
Conversation
36e62d2
to
3ffc6bd
Compare
13b0912
to
b329879
Compare
Contract, | ||
ContractConstructor, | ||
ContractCaller, | ||
) |
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.
These are the only classes that get exported to other files in our codebase. I think that our public API all stems from either Contract
or AsyncContract
, but I wonder if it's worth exposing other classes like ContractFunctions
or ContractEvents
here too?
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 would lean towards only exposing what is necessary and add more if when it's called for. Adding things here wouldn't be breaking, right?
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.
587a894
to
252b355
Compare
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.
LGTM! Nit/question - I've been grouping async functions together in the lower half of a file, separated with # --- async --- #
. Do we want to continue that theme here? I think it's cleaner in an overall organization sense, but there's merit to having sync and async versions visually next to each other if they need changes in the future. 🤷
oops, yes! Thanks for the reminder! |
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 lgtm! I'd piggy back what @pacrob said about splitting out the async functions and say that might be useful to do inside the [oops that's the only file I see for that hah]. But that's a nit.utils.py
file as well for better readability
69a8d04
to
2df6140
Compare
How was it fixed?
Reorganized contract to align with the format discussed in #2441
Todo: