feat: Add upgradeability features to SpokePool #208
Conversation
contracts/Ethereum_SpokePool.sol
Outdated
| @@ -25,7 +29,7 @@ contract Ethereum_SpokePool is SpokePool, Ownable { | |||
| **************************************/ | |||
|
|
|||
| function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override { | |||
| IERC20(relayerRefundLeaf.l2TokenAddress).transfer(hubPool, relayerRefundLeaf.amountToReturn); | |||
| IERC20(relayerRefundLeaf.l2TokenAddress).safeTransfer(hubPool, relayerRefundLeaf.amountToReturn); | |||
There was a problem hiding this comment.
Can we put this in a separate PR since it's pretty distinct from the pause functionality?
There was a problem hiding this comment.
yea I'm gonna turn this into a draft PR
nicholaspai
changed the title
nicholaspai
changed the title
| */ | ||
|
|
||
| interface SpokeAdapterInterface { | ||
| function bridgeTokensToHubPool(uint256 amountToReturn, address l2TokenAddress) external; |
There was a problem hiding this comment.
This is really cool! I think this is a huge reduction in the chain-specific lines in each SpokePool implementation.
Do you think it's possible to remove all chain-specific functionality from the SpokePools?
I was thinking that it would be really nice to be able to deploy the exact same SpokePool implementation to each chain along with an adapter that contains all the specialized functionality for: receiving messages from the hub, sending tokens to the HubPool, and anything else that might be specialized based on the chain.
To do this and handle all necessary functionality, the adapter will have to receive a lot of chain-specific function calls. I think this would ultimately either look like one of the two following architectures:
-
All the common SpokePool methods, like deposits and fills will have implementations in the main SpokePool. All specialized or customized functionality would live in the adapter. Any call that hits the SpokePool would be forwarded (via delegatecall) to the adapter by the fallback function if the SpokePool didn't implement it. Note: this is very very tricky from a permissioning perspective, since any methods on the adapter that are only meant to be called by the SpokePool would need to be protected from unauthorized forwards.
-
A full-on proxy where the entire SpokePool implementation sits behind a Proxy, and that implementation can be swapped out by governance. This is a more common pattern and would probably be safer, but gives governance full control over the implementation rather than just controlling some methods.
Thoughts on this general idea? Note: this gets complicated quickly, so I could also see us sticking with only making the token bridging upgradable.
There was a problem hiding this comment.
After our discussion offline I think we should favor going with a full upgradeable proxy pattern. The UUPS seems like a good one for our use case. We'd get the benefit of a well supported and tested proxy with less code change than this PR which refactors the contracts themselves to only have a tiny surface area be upgradeable.
It seems like the main change we'd have to make is to remove the constructor in favor of an initializer function, and generally inherit the Intializable contract
There was a problem hiding this comment.
100% agreed! I also think this makes the spoke pool upgradability work much much lower lift and less to audit. +1
Adds "SpokeAdapter" contracts that can be reset by the admin of the SpokePool. This adapter contains the logic to send tokens back to HubPool, which can change overtime and for each token. This will reduce the chance of spoke pool upgrades in future.
The diff is large but it should be 90% copy and pasting
SpokePoollogic into theSpoke_Adapter