Implement TeleporterOwnerUpgradeable #92
Conversation
minghinmatthewlam
requested review from
michaelkaplan13,
cam-schultz,
gwen917,
geoff-vball,
bernard-avalabs and
feuGeneA
as code owners
| @@ -637,11 +637,18 @@ contract ERC20BridgeTest is Test { | |||
| } | |||
|
|
|||
| function testUpdateMinTeleporterVersion() public { | |||
There was a problem hiding this comment.
Curious if you think it'd make sense to remove this test case from ERC20BridgeTests given that the implementation is tested in TeleporterOwnerUpgradeableTests. I don't think we commonly tests inherited functions in child contract test suites.
There was a problem hiding this comment.
hmm I'd usually agree, but I wanted at least one cross chain app that inherits TeleporterOwnerUpgradeable to have a unit test for it, and the other cross chain apps don't have their own unit tests. Also just thought extra unit tests don't hurt.
There was a problem hiding this comment.
A workaround to this issue would be to implement TeleporterOwnerUpgradable in a separate test contract, then move the unit test for testUpdateMinTeleporterVersion there.
There was a problem hiding this comment.
Oh, I see such a contract already exists. I agree with Michael that this unit test would make more sense in TeleporterUpgradeableTests.t.sol.
I don't think we sacrifice test coverage in doing so, since ERC20Bridge doesn't override this method. If it did, we'd definitely want to test it separately.
There was a problem hiding this comment.
that's true, since ERC20Bridge doesn't add additional override/functionality and just inherits, having the same test in TeleporterOwnerUpgradeableTests is already covering same test coverage. Removed.
There was a problem hiding this comment.
Thoughts on the same topic but for Ownable.transferOwnerhip and Ownable.renounceOwnership? I added extra unit tests for those in TeleporterOwnerUpgradeableTests though we just inherit the functions in TeleporterOwnerUpgradeable. Thinking behind it being these are external contracts, and we can have our own unit tests covering the integration with updateMinTeleporterVersion.
There was a problem hiding this comment.
👍
Yep that makes sense to me. The unit tests you're talking about that call transferOwnership and renounceOwnership are actually testing the behavior of updateMinTeleporterVersion, given its use of those other methods. Along that line, I would recommend keeping them in place as is, but possibly renaming them to "testCannotUpdateMinVersionAfterOwnershipTransfer", etc (or something along those lines).
| app.updateMinTeleporterVersion(); | ||
| } | ||
|
|
||
| function testRenounceOwnership() public { |
There was a problem hiding this comment.
Should we have a test case where renounceOwnership is called successfully, and then assert that the previous owner is unable to update the min teleporter version afterwards?
There was a problem hiding this comment.
yup the last few lines check this, but lmk if this should be made more clear or separated.
There was a problem hiding this comment.
Ah my bad, I was just reading it wrong before. 👍
One minor nit, can we have a MOCK_OWNER_ADDRESS value that we use for pranking & transferring ownership to in this test suite rather than using the teleporterAddress? I think it would make it slightly more clear and easier to reason about.
contracts/src/Teleporter/upgrades/tests/TeleporterOwnerUpgradeableTests.t.sol
Outdated
Show resolved
Hide resolved
| app.updateMinTeleporterVersion(); | ||
| } | ||
|
|
||
| function testRenounceOwnership() public { |
There was a problem hiding this comment.
Ah my bad, I was just reading it wrong before. 👍
One minor nit, can we have a MOCK_OWNER_ADDRESS value that we use for pranking & transferring ownership to in this test suite rather than using the teleporterAddress? I think it would make it slightly more clear and easier to reason about.
minghinmatthewlam
dismissed stale reviews from cam-schultz and bernard-avalabs
via
9fbde9e
minghinmatthewlam
dismissed stale reviews from cam-schultz and michaelkaplan13
via
bc1cd2c
minghinmatthewlam
requested review from
bernard-avalabs,
michaelkaplan13 and
cam-schultz
Why this should be merged
Fixes #75
How this works
New abstract contract
TeleporterOwnerUpgradeablethat inheritsTeleporterUpgradeableand implementsupdateMinTeleporterVersionso that only the owner of the contract can call the function. Replaces the previous cross chain application inheritances to use new contract.How this was tested
How is this documented
update README