feat(CCTP): ✨ modify adapters to include CCTP bridging of UDSC #391

james-a-morris · 2023-12-29T15:16:02Z

This PR creates a new CCTP Adapter that can be added to chain adapters to support bridging USDC on L1 -> L2 via the CCTP bridge. This PR also modifies the following chain adapters (both solidity code & deploy script code):

Optimism (closes ACX-1795)
Polygon (closes ACX-1792)
Arbitrum (closes ACX-1794)
Base (closes ACX-1793)

This commit creates a new CCTP Adapter that can be added to chain adapters to support bridging USDC on L1 -> L2 via the CCTP bridge

This hubpool mock is specfically designed to test chain-adapters through a delegate call. It allows for relaying messages and tokens.

linear · 2023-12-29T15:16:06Z

ACX-1795 Modify Optimism Chain Adapter

ACX-1792 Modify Polygon Chain Adapter

ACX-1794 Modify Arbitrum Chain Adapter

ACX-1793 Modify Base Chain Adapter

ACX-1791 Modify Chain Adapters to include the CCTP bridge

contracts/chain-adapters/Arbitrum_Adapter.sol

mrice32

Looks great -- I only have one question about variable storage, since it seems like we shouldn't need to store these in the proxy, itself, and we can keep them in the bytecode.

contracts/Arbitrum_SpokePool.sol

contracts/Ovm_SpokePool.sol

contracts/Polygon_SpokePool.sol

contracts/chain-adapters/Arbitrum_Adapter.sol

contracts/chain-adapters/Base_Adapter.sol

nicholaspai

Added some comments about storage slots and also doubled up matt's comment about constant state variables

nicholaspai · 2024-01-04T21:06:07Z

contracts/Ovm_SpokePool.sol

+     * @notice The official USDC contract address on this chain.
+     * @dev Posted officially here: https://developers.circle.com/stablecoins/docs/usdc-on-main-networks
+     */
+    IERC20 public l2Usdc;


if we're adding a new storage variable to a base contract like this one we need to decrement this contract's __gap variable. This contract is a base contract for Optimism/Base/Boba_SpokePool's. Decrement the __gap for each 32 byte slot used up: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

I've decremented the __gap by 2 slots to include space for the CircleCCTPAdapter's recipientCircleDomainId, usdcToken, and cctpTokenMessenger.

This is because: recipientCircleDomainId is 4 bytes, and usdcToken & cctpTokenMessenger should be 20 bytes each. So in total, 44 bytes fit in 2 slots.

contracts/Arbitrum_SpokePool.sol

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

nicholaspai · 2024-01-05T15:51:16Z

contracts/libraries/CircleCCTPAdapter.sol

+     */
+    function _transferUsdc(address to, uint256 amount) internal {
+        // Require that the CCTP bridge is enabled
+        require(_isCCTPEnabled(), "CCTP bridge is disabled");


In all of the SpokePools, we first check _isCCTPEnabled() before calling into here so the check here is redundant and adds gas. Can we remove this check and add a note to devs that this function will revert if cctpTokenMessenger is set incorrectly?

Although I'd prefer to have a readable failure - I agree that calling {null}.depositForBurn() is going to revert for us. I've removed the require() statement and added a dev warning: 7fb1f5b

nicholaspai · 2024-01-05T15:52:31Z

contracts/libraries/CircleCCTPAdapter.sol

+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import "../external/interfaces/CCTPInterfaces.sol";
+
+abstract contract CircleCCTPAdapter {


Nit: prefer naming this to CircleCCTP_SpokePool since its extended by SpokePools and not used as an L1 "Adapter" just because this would be confuinsg with the contracts in chain-adapters

Hmm agreed but could we use CircleCCTPConnector or something similar? The CircleCCTPAdapter is currently used by the chain adapters & the spoke pools since the logic is the same

nicholaspai · 2024-01-05T15:53:58Z

contracts/test/MockHubPool.sol

+    /// @dev This function delegates the call to the adapter contract to execute the balance check.
+    /// @param l2Token The address of the L2 token.
+    /// @param user The user address whose balance is being queried.
+    function balanceOf(address l2Token, address user) external {


why not just implement a more generalized function that takes in bytes memory l2CallData so that we can test with any arbitrary function? I think balanceOf is an interesting check but I wonder if there are additional test cases we'd want to send with state-modifyin xchain functions

Absolutely. I wrote this mock to include something related to arbitrary messaging but mostly for token exchange. I later tested the messaging with a hub pool deployment.

However, since we're including this in the repo, I'll update it to include arbitrary messaging.

Done: 42bdc15

nicholaspai · 2024-01-05T15:54:54Z

deploy/002_deploy_optimism_adapter.ts

      L1_ADDRESS_MAP[chainId].weth,
      L1_ADDRESS_MAP[chainId].optimismCrossDomainMessenger,
      L1_ADDRESS_MAP[chainId].optimismStandardBridge,
+      L1_ADDRESS_MAP[chainId].l1UsdcAddress,


nit: suggest commenting out the cctpTokenMessenger line and adding in a zeroAddress for production so we don't accidentally deploy this with full cctp support yet

This comment applies to all deploy scripts as well

Good call - per the sync today I've set all spoke/adapters cctpTokenMessenger contract to ZERO_ADDRESS.

Done here: 9a25b3b

nicholaspai · 2024-01-05T15:58:03Z

contracts/Arbitrum_SpokePool.sol

-            "" // _data. We don't need to send any data for the bridging action.
-        );
+        // If the l2TokenAddress is UDSC, we need to use the CCTP bridge.
+        if (_isCCTPEnabledForToken(l2TokenAddress)) {


WDYT about calling _isCCTPEnabled() && l2TokenAddress == usdcToken? I think its more intuitive what's going on here rather than hiding the second part of that check behind an internal function

I think it's a good call. I'm not sure how much additional readability it would give for someone familiar with the code, but it does bring the boolean check directly into where the bridging is called.

As a tradeoff - this change requires propagating the _isCCTPEnabled() && l2TokenAddress == usdcToken 8 times.

Commit: 318afa2

nicholaspai

This makes a lot of sense, I left some suggestions

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

mrice32

I like this design a lot -- just a few small comments!

mrice32 · 2024-01-05T22:48:09Z

contracts/Ovm_SpokePool.sol

    // affecting the storage layout of child contracts. Decrement the size of __gap whenever state variables
    // are added. This is at bottom of contract to make sure its always at the end of storage.
-    uint256[1000] private __gap;
+    uint256[998] private __gap;


What is this change? Did we add variables to the OVM SpokePool that would move storage slots?

My understanding is that since this class' inherited class was updated (the CircleCCTPAdapter) we'd need to modify this. In both cases, we didn't have any upgrade errors as outlined here: https://docs.openzeppelin.com/contracts/3.x/upgradeable#:~:text=You%20may%20notice%20that%20every,storage%20compatibility%20with%20existing%20deployments.

If the variables are constant or immutable then they don't take slots and this shouldn't be decremented

mrice32 · 2024-01-05T22:53:09Z

contracts/chain-adapters/Arbitrum_Adapter.sol

+     * @dev This identifier is assigned by Circle and is not related to a chain ID.
+     * @dev Official domain list can be found here: https://developers.circle.com/stablecoins/docs/supported-domains
+     */
+    uint32 public constant circleDomainId = 3;


nit: since this is only used to pass up through the constructor stack, I would suggest making it a private variable and name it ARBITRUM_CIRCLE_DOMAIN_ID (I think it's good to include the chain name so it's very clear that the constant is chain specific). It will be exposed via the recipientCircleDomainId in the parent class, so we don't really gain anything from exposing with a public function here.

Same for the other adapters.

Updated here: dd5e3f8

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32

LGTM!

james-a-morris added 6 commits December 29, 2023 10:09

feat(CCTP): ✨ add CCTP Adapter to easily onboard new chains

7634a34

This commit creates a new CCTP Adapter that can be added to chain adapters to support bridging USDC on L1 -> L2 via the CCTP bridge

feat(CCTP): ✨ add CCTP support for optimism

ee9b167

feat(CCTP): ✨ Add CCTP bridge support for Arbitrum L1 -> L2

3cb71c7

feat(CCTP): ✨ Add CCTP bridge support for Polygon L1 -> L2

5470b67

feat(CCTP): ✨ Add CCTP bridge support for Base L1 -> L2

0efe591

test(CCTP): 🤡 add mock for hubpool

7b2a113

This hubpool mock is specfically designed to test chain-adapters through a delegate call. It allows for relaying messages and tokens.

james-a-morris changed the base branch from master to cctp-stage December 29, 2023 15:17

james-a-morris marked this pull request as draft December 29, 2023 15:21

nicholaspai reviewed Dec 29, 2023

View reviewed changes

contracts/chain-adapters/Arbitrum_Adapter.sol Outdated Show resolved Hide resolved

james-a-morris requested a review from nicholaspai December 29, 2023 15:27

james-a-morris added 9 commits December 29, 2023 15:17

improve: hardcode cctp domains

83c2d22

test(CCTP): add chain-adapter specific tests

279c1d7

chore: refactor location of cctp adapter

2197c95

improve: remove inherited class

4e21feb

improve: change lib name

810c4af

improve: integrate CircleCCTPLib into spokes

e05f969

chore: change name of cctp library

a408b5d

improve: merge OVM logic into OVM Spoke

a8e829d

improve: update arb to include transfer

90ac33a

james-a-morris requested a review from dohaki January 2, 2024 15:30

dohaki mentioned this pull request Jan 3, 2024

feat: add finalize cctp messages script #394

Merged

mrice32 reviewed Jan 4, 2024

View reviewed changes

nicholaspai reviewed Jan 4, 2024

View reviewed changes

james-a-morris added 2 commits January 4, 2024 17:59

improve: update constructor calls

742ae53

improve: update storage slots

51f7a04

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

james-a-morris force-pushed the james/acx-1791-modify-chain-adapters-to-include-the-cctp-bridge branch from 28ee6a6 to 51f7a04 Compare January 5, 2024 00:15

james-a-morris requested review from mrice32 and nicholaspai January 5, 2024 00:16

james-a-morris marked this pull request as ready for review January 5, 2024 00:19

nicholaspai reviewed Jan 5, 2024

View reviewed changes

james-a-morris added 5 commits January 5, 2024 12:30

improve(CCTP): save gas by removing check

953e291

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

improve(CCTP): save gas by removing check

7fb1f5b

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

improve(CCTP): make bridge check more intuitive

318afa2

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

chore(CCTP): lint

4e15c14

improve(CCTP): disable CCTP bridge

9a25b3b

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

james-a-morris requested a review from nicholaspai January 5, 2024 18:53

improve(CCTP): update mock hubpool

7b40144

Signed-off-by: james-a-morris <jaamorris@cs.stonybrook.edu>

james-a-morris force-pushed the james/acx-1791-modify-chain-adapters-to-include-the-cctp-bridge branch from 42bdc15 to 7b40144 Compare January 5, 2024 20:50

mrice32 reviewed Jan 5, 2024

View reviewed changes

improve(CCTP): use private variables

dd5e3f8

james-a-morris requested a review from mrice32 January 6, 2024 22:18

chore: merge

9fae5c7

james-a-morris changed the base branch from cctp-stage to master January 6, 2024 23:29

james-a-morris and others added 3 commits January 6, 2024 18:31

fix: revert __gap change

0351dd0

feat(scripts): add bytecode script

9992529

bytecode size limit

22141c9

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 force-pushed the james/acx-1791-modify-chain-adapters-to-include-the-cctp-bridge branch from 1be19b4 to fad4f92 Compare January 8, 2024 02:40

fix tests and ci

5036e8d

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 force-pushed the james/acx-1791-modify-chain-adapters-to-include-the-cctp-bridge branch from fad4f92 to 5036e8d Compare January 8, 2024 02:42

james-a-morris added 2 commits January 7, 2024 21:50

chore: merge

61c625f

fix(merge): merge issues

4c4d495

mrice32 approved these changes Jan 8, 2024

View reviewed changes

mrice32 merged commit cd04662 into master Jan 8, 2024

mrice32 deleted the james/acx-1791-modify-chain-adapters-to-include-the-cctp-bridge branch January 8, 2024 03:23

feat(CCTP): ✨ modify adapters to include CCTP bridging of UDSC #391

feat(CCTP): ✨ modify adapters to include CCTP bridging of UDSC #391

Uh oh!

Conversation

james-a-morris commented Dec 29, 2023

Uh oh!

linear bot commented Dec 29, 2023

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicholaspai 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!

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!

james-a-morris Jan 5, 2024 • 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!

james-a-morris Jan 5, 2024 • 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!

nicholaspai 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

james-a-morris Jan 5, 2024 •

edited

Loading

james-a-morris Jan 5, 2024 •

edited

Loading