Skip to content
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

Upgradeability for TeleporterUpgradeable and TeleporterOwnerUpgradeable #397

Merged
merged 53 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
4699585
forge install: openzeppelin-contracts-upgradeable
cam-schultz May 23, 2024
1f9e9ec
upgradeable contracts are upgradeable
cam-schultz May 23, 2024
7ed72a1
Merge remote-tracking branch 'origin/main' into upgradeable-poc-merge
Jun 6, 2024
0e12905
reentrancy guard upgradeable and init ownable
Jun 6, 2024
530b3e6
rename init functions and SafeTransferFromUpgradeable
Jun 6, 2024
a322c6a
remove safe transferfrom upgradeable
Jun 6, 2024
175cc6f
Merge remote-tracking branch 'origin/main' into upgradeable-poc-merge
Jun 21, 2024
1678786
update TestMessenger
Jun 21, 2024
efed26d
Merge remote-tracking branch 'origin/main' into upgradeable-poc-merge
Jul 8, 2024
6f7a152
update tests to be compatible
Jul 8, 2024
697a555
update bindings
Jul 8, 2024
df1360a
update TestMessenger initializer
Jul 8, 2024
dc8ab61
lint fix
Jul 8, 2024
35f3fe8
namespace storage addition
Jul 9, 2024
1bc3317
update storage location
Jul 9, 2024
e23962e
add todo and bindings
Jul 11, 2024
d338985
update gitmodule and dependency
Jul 11, 2024
ed11c50
forge install: openzeppelin-contracts
Jul 11, 2024
8b8f896
upgrade oz-contracts to v5
Jul 11, 2024
ad5d23d
forge install: openzeppelin-contracts-upgradeable
Jul 11, 2024
36742d5
fix custom errors in unit tests
Jul 11, 2024
8727ac3
update versions to 5.0.2 for remapping
Jul 11, 2024
825edc3
bindings update
Jul 11, 2024
c026b9a
lint fixes
Jul 12, 2024
b43ff79
bindings update
Jul 12, 2024
151fdac
Merge branch 'namespace-storage' into version-updates
Jul 12, 2024
96c2bc2
lint fixes
Jul 16, 2024
d93e577
remove error string deu to custom errors
Jul 16, 2024
f63e8ea
change to tx revert error string
Jul 16, 2024
5504e95
fix file path
Jul 16, 2024
4bc9618
Merge remote-tracking branch 'origin/main' into upgradeable-poc-merge
Jul 16, 2024
c4a6042
bindings update
Jul 16, 2024
9128448
Merge branch 'upgradeable-poc-merge' into namespace-storage
Jul 16, 2024
27ac422
Merge branch 'namespace-storage' into version-updates
Jul 16, 2024
79736e5
additional merge updates
Jul 16, 2024
4db07d7
Merge pull request #426 from ava-labs/version-updates
Jul 18, 2024
5d0c6d9
Merge pull request #420 from ava-labs/namespace-storage
Jul 18, 2024
d3b0600
Merge branch 'main' into upgradeable-poc-merge
Jul 18, 2024
c60dbed
update to 0.8.23
Jul 18, 2024
85987c5
bindings update
Jul 18, 2024
15107fc
format and bindings
Jul 18, 2024
164b5ad
Merge remote-tracking branch 'origin/main' into upgradeable-poc-merge
Jul 19, 2024
fcf6012
update to use init functions
Jul 19, 2024
38acbea
increase deadline
Jul 24, 2024
050c3c1
private isTeleporterAddressPaused
Jul 24, 2024
cf01336
pr fixes and log forge version
Jul 24, 2024
dc384ad
revert e2e deadline
Jul 24, 2024
39344d4
ci quotes
Jul 24, 2024
3caccc6
update comment
Jul 25, 2024
b033a2a
explicit remapping for erc4626
Jul 25, 2024
c87e61b
no bytecode hash generation and pr nits
Jul 25, 2024
5f317b7
change comment
Jul 25, 2024
9ce49e8
remove extra remapping
Jul 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,6 @@
[submodule "contracts/lib/subnet-evm"]
path = contracts/lib/subnet-evm
url = https://github.com/ava-labs/subnet-evm
[submodule "contracts/lib/openzeppelin-contracts-upgradeable"]
path = contracts/lib/openzeppelin-contracts-upgradeable
url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable
minghinmatthewlam marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion contracts/remappings.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@avalabs/[email protected]/=lib/subnet-evm/contracts/
@openzeppelin/[email protected]=lib/openzeppelin-contracts/contracts/
@openzeppelin/[email protected]=lib/openzeppelin-contracts-upgradeable/contracts/
@teleporter/=src/Teleporter/
@mocks/=src/Mocks/
@mocks/=src/Mocks/
13 changes: 7 additions & 6 deletions contracts/src/Teleporter/tests/TestMessenger.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import {TeleporterMessageInput, TeleporterFeeInfo} from "@teleporter/ITeleporter
import {SafeERC20TransferFrom, SafeERC20} from "@teleporter/SafeERC20TransferFrom.sol";
import {TeleporterOwnerUpgradeable} from "@teleporter/upgrades/TeleporterOwnerUpgradeable.sol";
import {IERC20} from "@openzeppelin/[email protected]/token/ERC20/IERC20.sol";
import {ReentrancyGuard} from "@openzeppelin/[email protected]/security/ReentrancyGuard.sol";
import {ReentrancyGuardUpgradeable} from
"@openzeppelin/[email protected]/security/ReentrancyGuardUpgradeable.sol";

/**
* THIS IS AN EXAMPLE CONTRACT THAT USES UN-AUDITED CODE.
Expand All @@ -20,7 +21,7 @@ import {ReentrancyGuard} from "@openzeppelin/[email protected]/security/Reentrancy
* @dev TestMessenger is test fixture contract that exercises sending and receiving
* messages cross chain.
*/
contract TestMessenger is ReentrancyGuard, TeleporterOwnerUpgradeable {
contract TestMessenger is ReentrancyGuardUpgradeable, TeleporterOwnerUpgradeable {
minghinmatthewlam marked this conversation as resolved.
Show resolved Hide resolved
using SafeERC20 for IERC20;

// Messages sent to this contract.
Expand Down Expand Up @@ -50,10 +51,10 @@ contract TestMessenger is ReentrancyGuard, TeleporterOwnerUpgradeable {
bytes32 indexed sourceBlockchainID, address indexed originSenderAddress, string message
);

constructor(
address teleporterRegistryAddress,
address teleporterManager
) TeleporterOwnerUpgradeable(teleporterRegistryAddress, teleporterManager) {}
constructor(address teleporterRegistryAddress, address teleporterManager) {
__ReentrancyGuard_init();
__TeleporterOwnerUpgradeable_init(teleporterRegistryAddress, teleporterManager);
}

/**
* @dev Sends a message to another chain.
Expand Down
11 changes: 7 additions & 4 deletions contracts/src/Teleporter/upgrades/TeleporterOwnerUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,22 @@
pragma solidity 0.8.18;

import {TeleporterUpgradeable} from "./TeleporterUpgradeable.sol";
import {Ownable} from "@openzeppelin/[email protected]/access/Ownable.sol";
import {OwnableUpgradeable} from
"@openzeppelin/[email protected]/access/OwnableUpgradeable.sol";

/**
* @dev Contract that inherits {TeleporterUpgradeable} and allows
* only owners of the contract to update the minimum Teleporter version.
*
* @custom:security-contact https://github.com/ava-labs/teleporter/blob/main/SECURITY.md
*/
abstract contract TeleporterOwnerUpgradeable is TeleporterUpgradeable, Ownable {
constructor(
abstract contract TeleporterOwnerUpgradeable is TeleporterUpgradeable, OwnableUpgradeable {
function __TeleporterOwnerUpgradeable_init(
address teleporterRegistryAddress,
address initialOwner
) TeleporterUpgradeable(teleporterRegistryAddress) {
) internal onlyInitializing {
__TeleporterUpgradeable_init(teleporterRegistryAddress);
__Ownable_init();
transferOwnership(initialOwner);
}

Expand Down
20 changes: 15 additions & 5 deletions contracts/src/Teleporter/upgrades/TeleporterUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ pragma solidity 0.8.18;
import {TeleporterRegistry} from "./TeleporterRegistry.sol";
import {ITeleporterReceiver} from "../ITeleporterReceiver.sol";
import {ITeleporterMessenger, TeleporterMessageInput} from "../ITeleporterMessenger.sol";
import {Context} from "@openzeppelin/[email protected]/utils/Context.sol";
import {ReentrancyGuard} from "@openzeppelin/[email protected]/security/ReentrancyGuard.sol";
import {ContextUpgradeable} from
"@openzeppelin/[email protected]/utils/ContextUpgradeable.sol";
import {ReentrancyGuardUpgradeable} from
"@openzeppelin/[email protected]/security/ReentrancyGuardUpgradeable.sol";
import {SafeERC20} from "@openzeppelin/[email protected]/token/ERC20/utils/SafeERC20.sol";
import {IERC20} from "@openzeppelin/[email protected]/token/ERC20/IERC20.sol";

Expand All @@ -23,11 +25,15 @@ import {IERC20} from "@openzeppelin/[email protected]/token/ERC20/IERC20.sol";
*
* @custom:security-contact https://github.com/ava-labs/teleporter/blob/main/SECURITY.md
*/
abstract contract TeleporterUpgradeable is Context, ITeleporterReceiver, ReentrancyGuard {
abstract contract TeleporterUpgradeable is
ContextUpgradeable,
ITeleporterReceiver,
ReentrancyGuardUpgradeable
{
using SafeERC20 for IERC20;

// The Teleporter registry contract manages different Teleporter contract versions.
TeleporterRegistry public immutable teleporterRegistry;
TeleporterRegistry public teleporterRegistry;
/**
* @dev A mapping that keeps track of paused Teleporter addresses.
*/
Expand Down Expand Up @@ -60,7 +66,11 @@ abstract contract TeleporterUpgradeable is Context, ITeleporterReceiver, Reentra
* @dev Initializes the {TeleporterUpgradeable} contract by getting `teleporterRegistry`
* instance and setting `_minTeleporterVersion`.
*/
constructor(address teleporterRegistryAddress) {
function __TeleporterUpgradeable_init(address teleporterRegistryAddress)
internal
onlyInitializing
{
__ReentrancyGuard_init();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own understanding, do you know why ContextUpgradeable doesn't have a __ContextUpgradeable_init function to be called here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContextUPgradeable has no state to initialize with so the init functions are empty

    function __Context_init() internal onlyInitializing {
    }

    function __Context_init_unchained() internal onlyInitializing {
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, should we include calls to that here for completeness? In the event that state is added to those contracts in a future version I image we'd want to call it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still need to follow up this. If you include the Context init call, for new versions we'd need to check through the inheritance hierarchy to make sure there's no duplicates of initialize calls anyways. Likely if called, we can call the init_unchained call. Also was looking at OZ's OwnableUpgradeable which also inherits ContextUpgradeable but they do not call the Context init function, so might ask OZ about that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following convention in #397 (comment) to call init instead of unchained when not needed, went forward to call Context_init as well for completeness, and no duplicate initializations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related OZ issue with their upgradeable contract transpiler not always calling the init functions of parent contracts OpenZeppelin/openzeppelin-contracts#4690

require(
teleporterRegistryAddress != address(0),
"TeleporterUpgradeable: zero teleporter registry address"
Expand Down
Loading