-
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
Replace ConciseContract with ContractCaller #1025
Comments
Should ContractReader have access to ContractEvents? In my opinion - yes, because events are also part of reading contract data from blockchain. |
Hm, I'm not sure we'd be making the events API any easier to call. Maybe name it |
I like ContractCaller. So the idea is that ContractCaller can only make calls, and removing the ability to use modifiers to make transactions or estimate gas. For example, this would not be possible:
I was confused by the name ContractReader, which I thought could be an api to read public variables. |
for @kclowes
|
Note that's a slight change from the API in the OP: I think I like the OP API better, but highly open to persuasion or alternatives |
@carver I'm inclined to do something like this.
And maybe to alias |
The non-parenthetical API option looks good to me. 👍 One-letter variables would be super out-of-character for the tool suite. I think I could get behind it (same for ... or maybe just not have the alias. Maybe we could shorten |
Lets just leave out the alias. |
ConciseContract
shouldn't be a factory, it should be it's own calling API. meta-tracking in #722How can it be fixed?
Add deprecation warning to ConciseContract and ImplicitContract. Add a new api: ContractReader.
one proposed api:
Note that there will be internal breakage when removing
ConciseContract
, notably in theens
module. So part of this change will be updating ENS to use the new reader API. Brief discussion at #1118 (comment)The text was updated successfully, but these errors were encountered: