Change admin command #47
Conversation
| updateProxy({ package: proxyPackage, contract: proxyContract, address: proxyAddress }, fn) { | ||
| const fullname = toContractFullName(proxyPackage, proxyContract) | ||
| const index = _.findIndex(this.data.proxies[fullname], { address: proxyAddress }) | ||
| if (index === -1) throw Error(`Proxy ${fullname} at ${proxyAddress} not found in network manifest`) |
There was a problem hiding this comment.
did we already introduce the word manifest (documentation and logging lines)? otherwise, I'd stay to files
| @@ -148,6 +164,28 @@ export default class NetworkAppController extends NetworkBaseController { | |||
| log.error(`Possible migration method 'migrate' found in contract ${contractClass.contractName}. Remember running the migration after deploying it.`); | |||
| } | |||
|
|
|||
| _fetchOwnedProxies(packageName, contractAlias, proxyAddress) { | |||
There was a problem hiding this comment.
is there a need to specify owned? is there a chance for a project to manage other proxies than the ones it owns?
There was a problem hiding this comment.
After changing ownership of a proxy, the project will still keep track of it, and they will still be listed in the network file. It doesn't manage them, but it does have a reference to them.
| }) | ||
|
|
||
| if (_.isEmpty(proxies)) { | ||
| log.info('No owned contract instances that match were found'); |
There was a problem hiding this comment.
Shouldn't this be No matching contract instances were found?
We could also log what we were looking for to be clearer
There was a problem hiding this comment.
Note that you are repeating this validation in line 181, I think you can just delete this one since the filter above won't bother you in case no proxies were found at this point
There was a problem hiding this comment.
Validation in line 181 is about ownedProxies, not proxies, and I wanted to show different messages depending on whether no proxies were found, or those found were not owned by the app.
| _.map(proxies, async (proxy) => { | ||
| log.info(`Changing admin of proxy ${proxy.address} to ${newAdmin}`) | ||
| await this.project.changeProxyAdmin(proxy.address, newAdmin) | ||
| this.networkFile.updateProxy(proxy, proxy => ({ ... proxy, admin: newAdmin })) |
There was a problem hiding this comment.
shouldn't we set this value also when creating a new instance?
There was a problem hiding this comment.
We can safely assume that by default the admin of a new instance is the app
| } | ||
| async setProxiesAdmin(packageName, contractAlias, proxyAddress, newAdmin) { | ||
| const proxies = this._fetchOwnedProxies(packageName, contractAlias, proxyAddress) | ||
| if (proxies.length === 0) return []; |
There was a problem hiding this comment.
can we use lodash here to be consistent?
|
|
||
| if (_.isEmpty(ownedProxies)) { | ||
| log.info('No matching contract instances are owned by this application'); | ||
| return []; |
There was a problem hiding this comment.
I think there is no need to specify different returns giving the one above will work as you are expecting in case no proxies were found
|
|
||
| const name = 'set-admin' | ||
| const signature = `${name} [alias-or-address] [new-admin-address]` | ||
| const description = 'change upgradeability admin of a contract instance. Provide the [alias] or [package]/[alias] you added your contract with, or its [address]. Note that if you transfer to an incorrect address, you may irreversibly lose control over upgrading your contract.' |
There was a problem hiding this comment.
I would make clear that in case of an alias we are going to set the admin of all the instances for that alias
* Set admin script to change admin of a proxy * Register set-admin command * Add logging
* Set admin script to change admin of a proxy * Register set-admin command * Add logging
Add new
set-admincommand to change admin ownership of a proxy. The command requires a-yor--yesflag to be actually run, otherwise it cowardly refuses. It can be run either with an address, or a contract name (optionally namespaced by a package). For instance:Fixes #3