!feat: Make SpokePools upgradeable #218

nicholaspai · 2023-01-04T22:53:58Z

This PR makes SpokePools compatible with OpenZeppelin's upgradeable proxy pattern. Some important changes to support upgradeability:

SpokePool's constructor adds the _disableInitializers call to prevent it from being left uninitialized.
All non-constant state variables that have initial values are instead set in the initialize function.
Contracts that the SpokePool extends such as Lockable and Testable that have constructors replaces those with upgradeable versions that replace constructors with initializers.
All unit tests involving the spoke pool are changed to interface with an upgradeable version of the spoke pool

This PR makes SpokePools compatible with OpenZeppelin's upgradeable proxy pattern

linear · 2023-01-04T22:54:00Z

ACX-520 Add features to SpokePool to make future upgrades easier

Make SpokePool compatible with OZ upgradeable proxies

UUPS proxies allow the implementation contract to define the conditions under which an upgrade can be called, compared to a Transparent proxy in which only the owner of the proxy can upgrade. This allows the SpokePool upgrades to be triggered by the cross domain admin via the HubPool

into nick/acx-520-add-features-to-spokepool-to-make-future

…use no contracts use delegatecall() anymore

mrice32

LGTM!

mrice32 · 2023-01-31T07:09:12Z

contracts/upgradeable/LockableUpgradeable.sol

@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: AGPL-3.0-only


As discussed IRL, maybe we should just use the OZ lockable contract? Do you see any problem with that?

Nope, I looked into Lockable. The main feature we add is the _startReentrantGuardDisabled internal function but that's not used by Across contracts so I think we're safe to use OZ lockable

mrice32 · 2023-01-31T07:10:53Z

contracts/upgradeable/MultiCallerUpgradeable.sol

+
+/**
+ * @title MockSpokePool
+ * @notice Logic is 100% copied from "@uma/core/contracts/common/implementation/MultiCaller.sol" but one


We should remember to exclude this contract from the audit scope considering the contract's logic has already been audited.

pxrl · 2023-01-31T09:02:40Z

deployments/deployments.json

-    "Arbitrum_Adapter": { "address": "0x937958992799bF4A9a656E6596FD10d7Da5c2216", "blockNumber": 14704380 },
+    "Arbitrum_Adapter": { "address": "0x29528780E29abb8Af95a5e5a125b94766987543F", "blockNumber": 16243583 },
+    "Arbitrum_RescueAdapter": { "address": "0xC6fA0a4EBd802c01157d6E7fB1bbd2ae196ae375", "blockNumber": 16233939 },


Seems unrelated - maybe split it out?

This was accidentally removed so i'm just adding it back here

It's preferable from a commit hygiene standpoint to split it out and submit it separately - at least so it's not ++confusing when someone looks back at the commit history and sees it modified as part of something totally unrelated. Just my 2c.

test/chain-specific-spokepools/Arbitrum_SpokePool.ts

test/gas-analytics/SpokePool.RelayerRefundLeafExecution.ts

contracts/SpokePool.sol

pxrl · 2023-01-31T10:20:09Z

contracts/SpokePool.sol

    // Any deposit quote times greater than or less than this value to the current contract time is blocked. Forces
    // caller to use an approximately "current" realized fee. Defaults to 10 minutes.
-    uint32 public depositQuoteTimeBuffer = 600;
+    uint32 public depositQuoteTimeBuffer;


It won't be possible to add any more state variables to the base SpokePool after deployment. Have we ruled out adding some placeholders, just in case we find a use for something else in future?

Great point 6a9f509

What do you and @mrice32 think about adding 50 storage slots for future upgrades?

Great call! Any reason not to make it way way bigger than we'd ever need? Like 1000? Just to be super safe

Here's a thought experiment. Imagine we wanted to support bridging with arbitrary data so that someone could bridge-and-swap. Say we had to upgrade the deposit() function (or add a new depositWithData function). Would this be possible with upgradeable proxies? How much of the extra storage slots would we need?

It depends on the implementation. If the data is just hashed into the deposit data and added to an event, no storage changes would be necessary.

If we needed to store extra info about each deposit, deposits would need extra storage, but they aren't stored continuously, so that can be done without pre-reserving slots.

In this implementation, what extra contiguous storage would you imagine needing?

I'm not sure, this was just a functionality that I discussed with Ryan about adding and wanted to make sure we're prepared for it. Basically bridge-and-swap would be a good UX addition

What if we needed to add a new Base contract function, does that take up slots?

Nope, functions don't take up slots or anything. They are just more bytecode in the implementation contract.

into nick/acx-520-add-features-to-spokepool-to-make-future

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

mrice32

LGTM!

mrice32

#218 (comment)

pxrl

Nice!

pxrl · 2023-02-01T21:41:24Z

@nicholaspai One final thought: the OVM_SpokePool is also a base contract. Not sure whether we want to reserve some slots there as well?

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

nicholaspai · 2023-02-02T21:39:56Z

@nicholaspai One final thought: the OVM_SpokePool is also a base contract. Not sure whether we want to reserve some slots there as well?

Great call, i've added 100 slots for OVM_SpokePool and updated SpokePool's __gap to 1000

cc @mrice32

into nick/acx-520-add-features-to-spokepool-to-make-future

mrice32

@nicholaspai One final thought: the OVM_SpokePool is also a base contract. Not sure whether we want to reserve some slots there as well?

Great call, i've added 100 slots for OVM_SpokePool and updated SpokePool's __gap to 1000

cc @mrice32

@nicholaspai why not just use a 1000 gap everywhere to give us tons of room and standardize? We have 2^256 slots to work with here, so there's no running out :).

mrice32

LGTM!

nicholaspai · 2023-02-03T19:44:02Z

@dohaki @mrice32 @pxrl I had to update this to take into account the new EIP712 contract that the SpokePool extends. This required changing that contract to be upgradeable with an initializer function instead of a constructor see this commit 22f5fbe#diff-8c305f0568345cd237252c531582ce424adc9c7c9151cfefe304fd389b874c59 !

mrice32

LGTM!

mrice32 · 2023-02-03T22:48:40Z

contracts/upgradeable/EIP712CrossChainUpgradeable.sol

+     * variables without shifting down storage in the inheritance chain.
+     * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
+     */
+    uint256[1000] private __gap;


Why is the gap at the bottom of the file below the functions? Shouldn't it be at the end of the list of variables, like in the other contracts?

I'm not sure this is how OZ adds their gaps so I was following along https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/proxy/utils/UUPSUpgradeable.sol#L111

Makes sense. Either way is totally fine! However, I think this PR has it done both ways. Ovm_SpokePool and SpokePool seem to have it at the top. As long as it's consistent, LGTM!

mrice32 · 2023-02-03T22:48:46Z

contracts/upgradeable/MultiCallerUpgradeable.sol

+     * variables without shifting down storage in the inheritance chain.
+     * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
+     */
+    uint256[1000] private __gap;


Same as above.

mrice32 · 2023-02-03T22:48:51Z

contracts/upgradeable/TestableUpgradeable.sol

+     * variables without shifting down storage in the inheritance chain.
+     * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
+     */
+    uint256[1000] private __gap;


Same as above.

dohaki

Awesome LGTM 👍 I mainly looked at the changes related to EIP712

mrice32

#218 (comment)

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

pxrl · 2023-02-08T23:23:33Z

contracts/upgradeable/EIP712CrossChainUpgradeable.sol

- * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.0/contracts/utils/cryptography/EIP712.sol
+ * https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/cryptography/EIP712Upgradeable.sol
 *
 * NOTE: Modified version that allows to build a domain separator that relies on a different chain id than the chain this


Suggested change

* https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/cryptography/EIP712Upgradeable.sol

* https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/cb0c1443f9b0aded3b08dfb3088f5ed5c9c72bea/contracts/utils/cryptography/EIP712Upgradeable.sol

Referring to master could be misleading in future, so it probably makes sense to refer to a specific version. I expect that we're just migrating from openzeppelin-contracts v4.8.0 to openzeppelin-contracts-upgradeable v4.8.0 - is that correct? (nb. the linked version is a permalink to https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/release-v4.8/contracts/utils/cryptography/EIP712Upgradeable.sol)

(FWIW I hate that it stretches off to infinity because the URL is so long...)

contracts/upgradeable/EIP712CrossChainUpgradeable.sol

pxrl

Would be nice to be more specific in the comment referring to the OZ EIP712Upgradable contract, but otherwise 👍

nicholaspai added 2 commits January 4, 2023 17:45

[WIP] feat: Make SpokePool's upgradeable

b9a38f8

This PR makes SpokePools compatible with OpenZeppelin's upgradeable proxy pattern

commit

d409fca

nicholaspai and others added 9 commits January 4, 2023 17:54

Delete unknown-31337.json

034e801

Update deployments.json

f4185df

Update SpokePool.sol

6db44cd

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

cfdf07e

into nick/acx-520-add-features-to-spokepool-to-make-future

Fix test

54e97c0

Update SpokePool.sol

681a192

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

0ee774e

into nick/acx-520-add-features-to-spokepool-to-make-future

Update SpokePool.Admin.ts

98b272b

nicholaspai marked this pull request as ready for review January 24, 2023 18:26

nicholaspai changed the title ~~[WIP] feat: Make SpokePool's upgradeable~~ !feat: Make SpokePool's upgradeable Jan 24, 2023

nicholaspai added 2 commits January 24, 2023 15:00

No longer require unsafeAllow: delegatecall when deploying proxy beca…

dd41c7b

…use no contracts use delegatecall() anymore

Fix gas tests

77b449a

nicholaspai requested review from mrice32 and pxrl January 29, 2023 18:43

mrice32 approved these changes Jan 31, 2023

View reviewed changes

pxrl reviewed Jan 31, 2023

View reviewed changes

test/chain-specific-spokepools/Arbitrum_SpokePool.ts Outdated Show resolved Hide resolved

pxrl reviewed Jan 31, 2023

View reviewed changes

test/gas-analytics/SpokePool.RelayerRefundLeafExecution.ts Show resolved Hide resolved

pxrl changed the title ~~!feat: Make SpokePool's upgradeable~~ !feat: Make SpokePools upgradeable Jan 31, 2023

pxrl reviewed Jan 31, 2023

View reviewed changes

contracts/SpokePool.sol Outdated Show resolved Hide resolved

pxrl reviewed Jan 31, 2023

View reviewed changes

nicholaspai and others added 5 commits January 31, 2023 10:59

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

c4c4771

into nick/acx-520-add-features-to-spokepool-to-make-future

Use OZ lockable

7e2b7ed

specify revert reasons

ba9e88f

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

Update contracts/SpokePool.sol

8adde77

Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>

add 50 32byte slots to base SpokePool

6a9f509

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

mrice32 approved these changes Jan 31, 2023

View reviewed changes

nicholaspai requested a review from mrice32 January 31, 2023 20:32

mrice32 reviewed Feb 1, 2023

View reviewed changes

pxrl approved these changes Feb 1, 2023

View reviewed changes

nicholaspai added 2 commits February 2, 2023 16:30

add __gap to ovm-spokepool

2932946

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

__gap private

c689fa2

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

nicholaspai added 2 commits February 2, 2023 16:40

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

ccec16e

into nick/acx-520-add-features-to-spokepool-to-make-future

Update SpokePool.sol

c0bb1b1

nicholaspai requested review from mrice32 and pxrl February 2, 2023 21:56

mrice32 reviewed Feb 2, 2023

View reviewed changes

Update Ovm_SpokePool.sol

b8fb099

nicholaspai requested a review from mrice32 February 3, 2023 01:53

mrice32 approved these changes Feb 3, 2023

View reviewed changes

merge

22f5fbe

nicholaspai requested a review from mrice32 February 3, 2023 19:43

nicholaspai requested a review from dohaki February 3, 2023 19:48

mrice32 approved these changes Feb 3, 2023

View reviewed changes

nicholaspai requested a review from mrice32 February 4, 2023 18:10

dohaki approved these changes Feb 6, 2023

View reviewed changes

mrice32 reviewed Feb 6, 2023

View reviewed changes

move __gap to top

1d8414b

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

pxrl reviewed Feb 8, 2023

View reviewed changes

contracts/upgradeable/EIP712CrossChainUpgradeable.sol Show resolved Hide resolved

pxrl approved these changes Feb 8, 2023

View reviewed changes

mrice32 merged commit c50eeb5 into master Feb 10, 2023

mrice32 deleted the nick/acx-520-add-features-to-spokepool-to-make-future branch February 10, 2023 20:42

	* https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/cryptography/EIP712Upgradeable.sol
	* https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/cb0c1443f9b0aded3b08dfb3088f5ed5c9c72bea/contracts/utils/cryptography/EIP712Upgradeable.sol

!feat: Make SpokePools upgradeable #218

!feat: Make SpokePools upgradeable #218

Uh oh!

Conversation

nicholaspai commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Jan 4, 2023

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pxrl Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

pxrl left a comment

Choose a reason for hiding this comment

Uh oh!

pxrl commented Feb 1, 2023

Uh oh!

nicholaspai commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Feb 3, 2023

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Jan 4, 2023 •

edited

Loading

pxrl Jan 31, 2023 •

edited

Loading

nicholaspai commented Feb 2, 2023 •

edited

Loading

pxrl Feb 8, 2023 •

edited

Loading

pxrl left a comment •

edited

Loading