-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement TeleporterOwnerUpgradeable #92
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
94123bf
initial TeleporterOwnerUpgradeable integration and tests
a271a11
update README
dfaae4e
only emit if version is changed
6bd6ce5
Merge remote-tracking branch 'origin/main' into owner-upgrade
1351d38
remove redundant test
9fbde9e
create mock invalid owner and fix lint
e3a3453
Merge remote-tracking branch 'origin/main' into owner-upgrade
bc1cd2c
add doc and trigger pr actions
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
contracts/src/Teleporter/upgrades/TeleporterOwnerUpgradeable.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // (c) 2023, Ava Labs, Inc. All rights reserved. | ||
| // See the file LICENSE for licensing terms. | ||
|
|
||
| // SPDX-License-Identifier: Ecosystem | ||
|
|
||
| pragma solidity 0.8.18; | ||
|
|
||
| import "./TeleporterUpgradeable.sol"; | ||
| import "@openzeppelin/contracts/access/Ownable.sol"; | ||
|
|
||
| /** | ||
| * @dev Contract that implements the {TeleporterUpgradeable} interface and allows | ||
| * only owners of the contract to update the minimum Teleporter version. | ||
| */ | ||
| abstract contract TeleporterOwnerUpgradeable is TeleporterUpgradeable, Ownable { | ||
| constructor( | ||
| address teleporterRegistryAddress | ||
| ) TeleporterUpgradeable(teleporterRegistryAddress) {} | ||
|
|
||
| /** | ||
| * @dev See {TeleporterUpgradeable-updateMinTeleporterVersion} | ||
| * | ||
| * Updates the minimum Teleporter version allowed for receiving on this contract | ||
| * to the latest version registered in the {TeleporterRegistry}. | ||
| * Restricted to only owners of the contract. | ||
| * Emits a {MinTeleporterVersionUpdated} event if the minimum Teleporter version | ||
| * was updated. | ||
| */ | ||
| function updateMinTeleporterVersion() external override onlyOwner { | ||
| uint256 oldMinTeleporterVersion = minTeleporterVersion; | ||
| minTeleporterVersion = teleporterRegistry.getLatestVersion(); | ||
| if (minTeleporterVersion > oldMinTeleporterVersion) { | ||
| emit MinTeleporterVersionUpdated( | ||
| oldMinTeleporterVersion, | ||
| minTeleporterVersion | ||
| ); | ||
| } | ||
| } | ||
| } |
103 changes: 103 additions & 0 deletions
103
contracts/src/Teleporter/upgrades/tests/TeleporterOwnerUpgradeableTests.t.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // (c) 2023, Ava Labs, Inc. All rights reserved. | ||
| // See the file LICENSE for licensing terms. | ||
|
|
||
| // SPDX-License-Identifier: Ecosystem | ||
|
|
||
| pragma solidity 0.8.18; | ||
|
|
||
| import "forge-std/Test.sol"; | ||
| import "../TeleporterOwnerUpgradeable.sol"; | ||
| import "./TeleporterUpgradeableTests.t.sol"; | ||
|
|
||
| contract ExampleOwnerUpgradeableApp is TeleporterOwnerUpgradeable { | ||
| constructor( | ||
| address teleporterRegistryAddress | ||
| ) TeleporterOwnerUpgradeable(teleporterRegistryAddress) {} | ||
| } | ||
|
|
||
| contract TeleporterOwnerUpgradeableTest is TeleporterUpgradeableTest { | ||
| ExampleOwnerUpgradeableApp public app; | ||
| address public constant MOCK_INVALID_OWNER_ADDRESS = | ||
| 0xd54e3E251b9b0EEd3ed70A858e927bbC2659587d; | ||
|
|
||
| function setUp() public virtual override { | ||
| TeleporterUpgradeableTest.setUp(); | ||
| app = new ExampleOwnerUpgradeableApp(address(teleporterRegistry)); | ||
| } | ||
|
|
||
| function testOwnerUpdateMinTeleporterVersion() public { | ||
| uint256 minTeleporterVersion = app.minTeleporterVersion(); | ||
|
|
||
| // Check that call to update minimum Teleporter version reverts for non-owners | ||
| vm.prank(MOCK_INVALID_OWNER_ADDRESS); | ||
| vm.expectRevert("Ownable: caller is not the owner"); | ||
| app.updateMinTeleporterVersion(); | ||
|
|
||
| // Check that minimum Teleporter version was not updated | ||
| assertEq(app.minTeleporterVersion(), minTeleporterVersion); | ||
|
|
||
| _addProtocolVersion(teleporterRegistry, teleporterAddress); | ||
|
|
||
| // Check that call to update minimum Teleporter version succeeds for owners | ||
| vm.prank(address(this)); | ||
| vm.expectEmit(true, true, true, true, address(app)); | ||
| emit MinTeleporterVersionUpdated( | ||
| minTeleporterVersion, | ||
| minTeleporterVersion + 1 | ||
| ); | ||
| app.updateMinTeleporterVersion(); | ||
|
|
||
| assertEq(app.minTeleporterVersion(), minTeleporterVersion + 1); | ||
| } | ||
|
|
||
| function testOwnerTransfer() public { | ||
| uint256 minTeleporterVersion = app.minTeleporterVersion(); | ||
| _addProtocolVersion(teleporterRegistry, teleporterAddress); | ||
|
|
||
| // Check that call to transfer ownership reverts for non-owners | ||
| vm.prank(MOCK_INVALID_OWNER_ADDRESS); | ||
| vm.expectRevert("Ownable: caller is not the owner"); | ||
| app.transferOwnership(address(0)); | ||
|
|
||
| // Check that ownership was not transferred | ||
| assertEq(app.owner(), address(this)); | ||
|
|
||
| // Check that call for non owners reverts | ||
| vm.prank(MOCK_INVALID_OWNER_ADDRESS); | ||
| vm.expectRevert("Ownable: caller is not the owner"); | ||
| app.updateMinTeleporterVersion(); | ||
|
|
||
| // Check that after ownership transfer call succeeds | ||
| address newOwner = address(0x123); | ||
| app.transferOwnership(newOwner); | ||
| vm.prank(newOwner); | ||
| vm.expectEmit(true, true, true, true, address(app)); | ||
| emit MinTeleporterVersionUpdated( | ||
| minTeleporterVersion, | ||
| minTeleporterVersion + 1 | ||
| ); | ||
| app.updateMinTeleporterVersion(); | ||
|
|
||
| // Check that call with old owner reverts | ||
| vm.expectRevert("Ownable: caller is not the owner"); | ||
| app.updateMinTeleporterVersion(); | ||
| } | ||
|
|
||
| function testRenounceOwnership() public { | ||
| // Check that call to renounce ownership reverts for non-owners | ||
| vm.prank(MOCK_INVALID_OWNER_ADDRESS); | ||
| vm.expectRevert("Ownable: caller is not the owner"); | ||
| app.renounceOwnership(); | ||
|
|
||
| // Check that ownership was not renounced | ||
| assertEq(app.owner(), address(this)); | ||
|
|
||
| // Check that update Teleporter version call for owner succeeds | ||
| app.updateMinTeleporterVersion(); | ||
|
|
||
| // Check that after ownership renounce call reverts | ||
| app.renounceOwnership(); | ||
| vm.expectRevert("Ownable: caller is not the owner"); | ||
| app.updateMinTeleporterVersion(); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a test case where
renounceOwnershipis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah my bad, I was just reading it wrong before. 👍
One minor nit, can we have a
MOCK_OWNER_ADDRESSvalue that we use for pranking & transferring ownership to in this test suite rather than using theteleporterAddress? I think it would make it slightly more clear and easier to reason about.