Fail with explicit error when creating proxy for missing stdlib #322
Conversation
spalladino
force-pushed
the
fix/clear-error-with-unpushed-stdlib-link
branch
from
cb5b228
to
7dd24c3
Compare
| isContractDefined(contractAlias) { | ||
| return this.packageFile.hasContract(contractAlias) | ||
| } | ||
|
|
||
| isContractDeployed(contractAlias) { | ||
| return !this.packageFile.hasContract(contractAlias) || this.networkFile.hasContract(contractAlias); | ||
| return !this.isLocalContract(contractAlias) || this.networkFile.hasContract(contractAlias); |
There was a problem hiding this comment.
I'd move all these logic somewhere else, half of the lines of this Controller are related to contract deployment verifications
There was a problem hiding this comment.
Not sure if I agree. All verifications require access to both package and network files (available at the controller), and are invoked by scripts which typically interact with the network controller directly. Furthermore, logic is different between app and lib, which matches nicely with the hierarchy of base/app/lib controllers.
I can give it a shot by moving this to a ContractChecker or DeploymentVerifier, but I doubt it will make the design more clear, as the Controller will need to pass-through most calls to it.
There was a problem hiding this comment.
Yes, I was sth like that... I'm ok having the controllers talking with other objects. I see a controller as an object that knows all the objects that have to be called in order to perform an action, right now ours are a mix between that and performing the requested action by themselves. I'm ok with moving this to an issue and discuss it later if you want :)
| @@ -111,48 +111,57 @@ export default class NetworkBaseController { | |||
| } | |||
|
|
|||
| checkLocalContractsDeployed(throwIfFail = false) { | |||
| let msg; | |||
| this._handleErrorMessage(this._errorForLocalContractsDeployed(), throwIfFail); | |||
There was a problem hiding this comment.
IMO checking and handling are kind of disruptive here, it is kind of weird to call a handler as a first step. I would have expected a condition at least.
There was a problem hiding this comment.
Agree, changed it
spalladino
force-pushed
the
fix/clear-error-with-unpushed-stdlib-link
branch
from
7dd24c3
to
7f51b2a
Compare
|
@facuspagnuolo implemented one of the changes, though I'm not sure about the other. I can give it a shot, but I'm not sure it's worthwhile atm. WDYT? |
changelog.md
Outdated
| ======= | ||
| - Fail with an explicit error when attempting to create a proxy for an stdlib that was linked but not pushed to the network ([#322](https://github.com/zeppelinos/zos-cli/pull/322)) | ||
|
|
||
| >>>>>>> Fail with explicit error when creating proxy for missing stdlib |
There was a problem hiding this comment.
I think you had already fixed it, a rebase from master should be enough
| isContractDefined(contractAlias) { | ||
| return this.packageFile.hasContract(contractAlias) | ||
| } | ||
|
|
||
| isContractDeployed(contractAlias) { | ||
| return !this.packageFile.hasContract(contractAlias) || this.networkFile.hasContract(contractAlias); | ||
| return !this.isLocalContract(contractAlias) || this.networkFile.hasContract(contractAlias); |
There was a problem hiding this comment.
Yes, I was sth like that... I'm ok having the controllers talking with other objects. I see a controller as an object that knows all the objects that have to be called in order to perform an action, right now ours are a mix between that and performing the requested action by themselves. I'm ok with moving this to an issue and discuss it later if you want :)
Creating a proxy for a contract that belongs to an stdlib that was linked, but that link was not pushed to the network, would yield a "VM Revert" error. Now it explains that the stdlib is linked but a push is missing.
spalladino
force-pushed
the
fix/clear-error-with-unpushed-stdlib-link
branch
from
7f51b2a
to
9bff8a0
Compare
…elinos#322) * Fail with explicit error when creating proxy for missing stdlib Creating a proxy for a contract that belongs to an stdlib that was linked, but that link was not pushed to the network, would yield a "VM Revert" error. Now it explains that the stdlib is linked but a push is missing. * Improve error handling in check methods of network base controller
…elinos/zos-cli#322) * Fail with explicit error when creating proxy for missing stdlib Creating a proxy for a contract that belongs to an stdlib that was linked, but that link was not pushed to the network, would yield a "VM Revert" error. Now it explains that the stdlib is linked but a push is missing. * Improve error handling in check methods of network base controller
Creating a proxy for a contract that belongs to an stdlib that was
linked, but that link was not pushed to the network, would yield
a "VM Revert" error. Now it explains that the stdlib is linked but
a push is missing.
Fixes #218