-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Support Register/List/Update Smart Contract #65
Conversation
🟡 Heimdall Review Status
|
cdp/smart_contract.py
Outdated
@@ -141,6 +156,16 @@ def deployer_address(self) -> str: | |||
""" | |||
return self._model.deployer_address | |||
|
|||
@property | |||
def is_external(self) -> str: |
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 should return a boolean
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.
Good catch updated
cdp/smart_contract.py
Outdated
|
||
""" | ||
return self._model.contract_name | ||
|
||
@property | ||
def deployer_address(self) -> str: |
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 think now a lot of these other properties will be only set if is_external is false.
This is the NodeJS change we had to make to support this:
coinbase/coinbase-sdk-nodejs@94680a6#diff-2c6bead48767df9b8a842ac4887f9e676b0b38a3b6fc2c4bdb7ff6e2d9ddbb7dR227
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 added a check of the external fields before returning. If it looks good I'll apply it to the nodejs change
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.
If it looks good I'll apply it to the nodejs change
I just linked the change that was made to the NodeJS to handle this!
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.
We'll also want to handle #sign, #wait, #broadcast, etc... to ensure they throw appropriate errors when called on external contracts
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.
Gotcha, updated! Thank you
Can you make sure your commits are getting verified? You may need git config scoped to this repo that includes your specific external GitHub username + signing key configuration |
Please rebase down to only verified commits |
280d634
to
fdcbd42
Compare
Thank you, just reabased all the previous commit into one |
@@ -373,6 +414,105 @@ def read( | |||
) | |||
return cls._convert_solidity_value(model) | |||
|
|||
@classmethod | |||
def update( |
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.
Update should be an instance method.
E.g. we've already registered or fetched a SmartContract resource that we then can call update on
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.
We can leave a class method helper, but we should be able to update off the actual contract resource.
smart_contract.update(name="New Name")
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.
Lets fast-follow with the instance method update function and add it to the Changelog!
What changed? Why?
Support Register/List/Update Smart Contract
Test:
Qualified Impact