Store deployment info #320
Conversation
Store deployment information for the current contracts version, per chain: - addresses - block numbers - transaction hashes - constructor parameters - deployment gas cost
- check that the `deployment_[CHAIN_NAME].json` file created post deployment contains correct information, compared to the chain - check that deployed bytecode matches the one from the compiled `contracts.json` file for each contract - add Readme information to run the verification separate from the deployment: `python -m raiden_contracts.deploy verify --rpc-provider http://127.0.0.1:8545`
loredanacirstea
force-pushed
the
store-deployment-info
branch
from
0bfea73 to
320c1dc
Compare
|
Reviewing this |
raiden_contracts/constants.py
Outdated
| @@ -98,7 +98,7 @@ class MessageTypeId(IntEnum): | |||
| START_QUERY_BLOCK_KEY = 'DefaultStartBlock' | |||
|
|
|||
|
|
|||
| class ChainId(Enum): | |||
| class ChainId(IntEnum): | |||
There was a problem hiding this comment.
As discussed in rocketchat perhaps let's remove this IntEnum and replace it with constants. Semantically this is only the known chain IDs. The user may provide a custom chain ID and then an enum no longer works. This already has bitten us in the client and had to go around it.
There was a problem hiding this comment.
I left only ID_TO_NETWORKNAME and NETWORKNAME_TO_ID. As far as I see, these should be enough.
| with deployment_file_path.open() as deployment_file: | ||
| deployment_data = json.load(deployment_file) | ||
| except (JSONDecodeError, UnicodeDecodeError) as ex: | ||
| raise DeploymentVerificationError(f'Cannot load deployment data file: {ex}') from ex |
There was a problem hiding this comment.
should this be {str(ex)}?
There was a problem hiding this comment.
@LefterisJP {ex} == {ex!s} == {str(ex)}, afaict
There was a problem hiding this comment.
@andrevmatos Is this an f-string thing? Or do we always wrap exceptions in str() in the client due to the struct logger? Cause in the client we do a lot of:
except RaidenRecoverableError as e:
log.error(str(e))
If we don't wrap it in str() there, then we get the TypeError: unsupported operand type(s) for +=: 'RaidenRecoverableError' and 'str'
But perhaps seems like it is a structlogger thing.
There was a problem hiding this comment.
Not sure about structlogger, but: https://www.python.org/dev/peps/pep-0498/#s-r-and-a-are-redundant, also supports lambdas etc.
raiden_contracts/deploy/__main__.py
Outdated
| # Check that the deployed bytecode matches the precompiled data | ||
| blockchain_bytecode = web3.eth.getCode(endpoint_registry_address).hex() | ||
| compiled_bytecode = contract_manager.contracts[CONTRACT_ENDPOINT_REGISTRY]['bin'] | ||
| # Compiled code contains some additional initial data compare to the blockchain bytecode |
| ChainId.RINKEBY: RINKEBY, | ||
| ChainId.KOVAN: KOVAN, | ||
| ChainId.SMOKETEST: SMOKETEST, | ||
| 1: 'mainnet', |
There was a problem hiding this comment.
But can you make them also constants?
e.g.:
NETWORKID_MAINNET = 1
NETWORKID_ROPSTEN = 3
...
e.t.c.
There was a problem hiding this comment.
Are they used somewhere? My thought was that ID_TO_NETWORKNAME and NETWORKNAME_TO_ID should be enough to do the job and I did not want redundant variables inside that file.
There was a problem hiding this comment.
It would allow us in the client to refer to ropsten's (or any other network's) ID programmatically without going through the ID_TO_NETWORKNAME whenever we want network specific behavior. It's just a convenience to avoid a mapping lookup.
And constant variables are not redundant in constants.py :)
There was a problem hiding this comment.
And constant variables are not redundant in constants.py :)
My problem is defining constants in raiden-contracts that are not used in raiden-contracts. If they are used only in raiden, then raiden should define them. Otherwise, we can break things by mistake.
To be honest, I would not even use raiden_contracts.ID_TO_NETWORKNAME in raiden... because of this. I would only stick to the raiden_contracts specific deliverables: contracts source, compiled files, ContractManager and the specific contracts-related constants.
But if you really want them added, I will.
There was a problem hiding this comment.
By the way, the entire ID_TO_NETWORK_CONFIG will be removed in the PR that solves #321
We will have json files with deployment data. So, any raiden specific network configs will be handled by raiden anyway.
The contracts will just provide a way to get deployment info for a specific version & chain.
There was a problem hiding this comment.
My problem is defining constants in raiden-contracts that are not used in raiden-contracts.
Last time we discussed I had understood you wanted contract deployment related data to be in contracts. It used to be in raiden constants.py but then got moved here.
I really have no strong opinion on this. Just finalize these as quick as possible so that the client code has time to adapt. Whatever you remove from here, we will have in the client.
The contracts will just provide a way to get deployment info for a specific version & chain.
So will the contracts package provide a method for us to read this json file or will we have to implement it in the client?
There was a problem hiding this comment.
Last time we discussed I had understood you wanted contract deployment related data to be in contracts
Correct. This has not changed. raiden_contracts will provide all contracts deployment info.
So will the contracts package provide a method for us to read this json file or will we have to implement it in the client?
I was thinking of adding in raiden_contracts a method for reading the json file based on version (default=current version) and the chain id, to make it easier for clients. Does this sound ok?
There was a problem hiding this comment.
a method for reading the json file based on version (default=current version) and the chain id, to make it easier for clients. Does this sound ok?
I think it does. These 2 pieces of information is what the client will have. At the moment we also have this development | release configuration as additional data. I guess we can incorporate that in the version?
There was a problem hiding this comment.
development | release
Not sure what this is about. Are you talking about the pre-limits version and current version of the contracts? For this, I will have 2 versioned data dirs with the compiled + deployment info. E.g. data_0.4.2 for current one, and data_[PRE_LIMITS_VERSION] (I'll check what version that was) or something similar (I have to recheck the bumpversion release process, to see how this would work)
There was a problem hiding this comment.
Are you talking about the pre-limits version and current version of the contracts?
Yes exactly.
Your proposal sounds fine then. And we can make the choice based on the version in the client then, using the tool that you provide.
fixes #306 (deployment script should write the deployment info in a json file)
fixes #312 (post-deployment checks)
Store deployment information for the current contracts version, per chain:
Add post-deployment verification
deployment_[CHAIN_NAME].jsonfile created post deployment contains correct information, compared to the chaincontracts.jsonfile for each contractpython -m raiden_contracts.deploy verify --rpc-provider http://127.0.0.1:8545Add Rinkeby deployment information. (Kovan & Ropsten will be added in another PR, as there are some more issues to solve)