ERC20Connector: add replay protection. Make AdminControlled. Add tests. Refactoring #33
Conversation
* Add replay protection after the possible upgrade by providing the `minBlockAcceptanceHeight` constraint for the proofs acceptance. * Rename `Locker` contract to `ProofKeeper`. * Minor improvements. * Refactoring.
* Add `AdminControlled` contract. * ERC20Locker: inherit from `AdminControlled`. * ERC20Locker: make `lockToken` and `unlockToken` pausable. * Add tests.
sept-en
changed the title
| import "rainbow-bridge/contracts/eth/nearprover/contracts/ProofDecoder.sol"; | ||
| import "rainbow-bridge/contracts/eth/nearbridge/contracts/Borsh.sol"; | ||
|
|
||
| contract ProofKeeper { |
There was a problem hiding this comment.
I'm not fully onboard with this new name. I understand your motivation for more generic connector that goes beyond locking tokens, but in this particular case Locker makes perfectly clear the goal of this contract while ProofKeeper doesn't IMO.
There was a problem hiding this comment.
Sure, I will split this into separate PRs (though 1 and 3 items will be in one as they are dependent).
What's about Locker - I believe that this name is truly misleading because actually, the contract locks nothing. And moreover, it contains just a _parseProof method and a map of used events and references the appropriate proof producer. So I don't think that old name doesn't represent the behavior of the contract.
On the other hand, the map which is inside - usedEvents (I believe that could be renamed to usedProofs because these are actually proofs, not events) - and _parseAndConsumeProof are there and the contract just keeps used proofs and is able to parse and consume proofs meanwhile verifying that this originates from the expected proof producer. And also as we already have ProofDecoder this doesn't look foreign IMO. But I agree that ProofKeeper isn't the perfect name as well. Perhaps, ProofConsumer or ProofController are a little bit better names but I'm not sure.
But if you think that it's better to keep old names for this connector, we can do this way.
| @@ -0,0 +1,42 @@ | |||
| pragma solidity ^0.6.12; | |||
|
|
|||
| contract AdminControlled { | |||
There was a problem hiding this comment.
Don't copy past this code and reuse the one from rainbow-bridge
There was a problem hiding this comment.
I'd like to but this part hasn't been merged yet.
| using ProofDecoder for Borsh.Data; | ||
|
|
||
| INearProver public prover_; | ||
| bytes public nearProofProducerAccount_; |
There was a problem hiding this comment.
Same with this name. While you are trying to use generic name, nearTokenFactory conveys better the idea of what is happening here IMO.
There was a problem hiding this comment.
The ERC20Locker passes the nearTokenFactory as an argument to this contract, so I think it already specifies how this will be used. Nevertheless, I'm ok to keep the old name if you think of this contract as specific to this connector.
No description provided.