Improved deployers tests#1111
Conversation
Codecov Report
@@ Coverage Diff @@
## sekanjabin #1111 +/- ##
==============================================
- Coverage 83.42% 83.26% -0.16%
==============================================
Files 71 71
Lines 9707 9747 +40
==============================================
+ Hits 8098 8116 +18
- Misses 1609 1631 +22
Continue to review full report at Codecov.
|
0de6912 to
2e26f57
Compare
| secret_hash = self.contract.functions.secretHash().call() | ||
| is_valid_secret = secret_hash == keccak_digest(secret) | ||
|
|
||
| if is_valid_secret: # Yay! \ (•◡•) / |
|
|
||
|
|
||
| class Deployer(NucypherTokenActor): | ||
| class DeployerActor(NucypherTokenActor): |
There was a problem hiding this comment.
It wants another name. Any ideas?
There was a problem hiding this comment.
Can it not stay Deployer?
Who is an actor which deploys a token or contract? Bursar? Secretary? Executive?
There was a problem hiding this comment.
@jMyles - This name clashes with the namespace in nucypher.blockchain.eth.deployers.Deployer.
|
|
||
| def __eq__(self, other) -> bool: | ||
| """Actors are equal if they have the same address.""" | ||
| return bool(self.checksum_address == other.checksum_address) |
There was a problem hiding this comment.
This will raise AttributeError is the other doesn't have a checksum_address. This might be OK, but also maybe we want to catch it and raise TypeError instead?
There was a problem hiding this comment.
My initial impression is that returning False is desirable is that situation.
|
|
||
|
|
||
| class Deployer(NucypherTokenActor): | ||
| class DeployerActor(NucypherTokenActor): |
There was a problem hiding this comment.
Can it not stay Deployer?
Who is an actor which deploys a token or contract? Bursar? Secretary? Executive?
nucypher/blockchain/eth/actors.py
Outdated
| paint_contract_deployment(contract_name=NucypherTokenDeployer.contract_name, | ||
| receipts=token_receipts, | ||
| contract_address=token_deployer.contract_address) | ||
| deployment_receipts[NucypherTokenDeployer.contract_name] = token_receipts |
There was a problem hiding this comment.
So, the key space of the dict will be values of NucypherTokenDeployer.contract_name... does this value mutate then? It's not obvious how to think about this.
There was a problem hiding this comment.
No - they do not mutate - they are hard-coded class attributes on the Deployer claas. This is an effort to have a single source of contract names in the codebase. In general, this method can be code-optimized somewhat with a loop and/or function.
There was a problem hiding this comment.
Maybe uhhh... 🤔... Maybe use a constant? NucypherTokenDeployer.contract_name is a hard thing for the reader to resolve in terms of understanding the keyspace here.
There was a problem hiding this comment.
Yeah I can see it that way. You propose we have a constants module with all the contract name strings defined?
| staker = Staker(is_me=True, checksum_address=checksum_address, blockchain=self.blockchain) | ||
| if not password: | ||
| raise ValueError(f'No checksum address or password supplied to initialize new stake.') | ||
| staker = self.__create_staker(password=password) |
There was a problem hiding this comment.
I need to dive in w/ a debugger, but I'm imagining that a random wallet is generated here? Maybe that needs to be called out in at least a comment on this layer?
There was a problem hiding this comment.
I agree - more comments are needed here. This area needs some thinking, ( see related Issue #1128 and #1129 ). In the case of a software wallet - the RPC endpoint for account creation is used. In the cased of a hardware wallet, an unused account is derived and used (which is always the last index geth loads for hardware wallets).
There was a problem hiding this comment.
Yeah, so this is a bit of a lava bridge. I think it's a little nerve-racking for self.__create_staker to, without affirmative direction to do so, use a heretofore unused checksum address.
There was a problem hiding this comment.
From a brid's-eye view does it make sense to implement the feature at all? (#1129 ). What's the user's experience like if they need to create accounts to stake with?
There was a problem hiding this comment.
@jMyles - Also, If you read above, the user is prompted for consent to use a new account.
New behavior is to look first for an existing Linker, and just in case it's not found, deploy it.
… expense of coinbase ¯\_(ツ)_/¯
|
@cygnusv your tests are so easy to read 🎉 ! |
tests/blockchain/eth/entities/deployers/test_user_escrow_deployer.py
Outdated
Show resolved
Hide resolved
KPrasch
left a comment
There was a problem hiding this comment.
Several follow up Issues opened, but looking good.
The goal of this PR is to improve the deployer tests. Initially, the goal included to improve actor tests (hence
drama-school), but I prefer to reduce the scope of the PR for the moment. Nevertheless, it does include some improvements in some actor tests (e.g., a rough test of Investigator).Hopefully, this will increase coverage a little.