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

feat(ink): configure and deploy Ink contracts #808

Merged
merged 15 commits into from
Dec 20, 2024

Conversation

james-a-morris
Copy link
Contributor

@james-a-morris james-a-morris commented Dec 18, 2024

All deployments made against origin commit hash 7f9ebb360c9bfcc9960a0226a92098fccc186ce8. Rebased against master with all deployments made.

Copy link

linear bot commented Dec 18, 2024

deploy/consts.ts Show resolved Hide resolved
contracts/Ink_SpokePool.sol Outdated Show resolved Hide resolved
@james-a-morris james-a-morris requested review from bmzig and pxrl December 19, 2024 18:37
@james-a-morris james-a-morris force-pushed the james/acx-3455-deploy-contracts branch from 1b13aed to 52e60b1 Compare December 19, 2024 19:53
@james-a-morris james-a-morris force-pushed the james/acx-3455-deploy-contracts branch from 52e60b1 to ed8842b Compare December 19, 2024 20:00
Copy link

socket-security bot commented Dec 19, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

View full report↗︎

@james-a-morris james-a-morris marked this pull request as ready for review December 19, 2024 20:01
{} // solhint-disable-line no-empty-blocks

/**
* @notice Construct the OVM World Chain SpokePool.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably just drop any chain-specific mentions here; they just cause unnecessary changes on each deployment.

Suggested change
* @notice Construct the OVM World Chain SpokePool.
* @notice Construct the SpokePool.

Comment on lines +56 to +57
* @dev This implementation deviates slightly from `_bridgeTokensToHubPool` in the `Ovm_SpokePool` contract since World Chain has a USDC bridge which uses
* a custom interface. This is because the USDC token on World Chain is meant to be upgraded to a native, CCTP supported version in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @dev This implementation deviates slightly from `_bridgeTokensToHubPool` in the `Ovm_SpokePool` contract since World Chain has a USDC bridge which uses
* a custom interface. This is because the USDC token on World Chain is meant to be upgraded to a native, CCTP supported version in the future.
* @dev This implementation deviates slightly from `_bridgeTokensToHubPool` in the `Ovm_SpokePool` contract since
* this chain uses Circle's bridged (upgradable to native) USDC standard, which uses a custom interface.

Comment on lines +60 to +62
// Handle custom USDC bridge which doesn't conform to the standard bridge interface. In the future, CCTP may be used to bridge USDC to mainnet, in which
// case bridging logic is handled by the Ovm_SpokePool code. In the meantime, if CCTP is not enabled, then use the USDC bridge. Once CCTP is activated on
// WorldChain, this block of code will be unused.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Handle custom USDC bridge which doesn't conform to the standard bridge interface. In the future, CCTP may be used to bridge USDC to mainnet, in which
// case bridging logic is handled by the Ovm_SpokePool code. In the meantime, if CCTP is not enabled, then use the USDC bridge. Once CCTP is activated on
// WorldChain, this block of code will be unused.
// Handle Circle's bridge for upgradable USDC, which doesn't conform to the standard OP bridge interface. In the future, CCTP may be used to bridge USDC to mainnet, in which
// case bridging logic is handled by the Ovm_SpokePool code. In the meantime, if CCTP is not enabled, then use the USDC bridge. Once CCTP is activated on this chain, this block of code will be unused.

Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

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

LGTM - we can generalise the SpokePool comments in a follow-up PR.

Copy link
Contributor

@bmzig bmzig left a comment

Choose a reason for hiding this comment

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

Should you also add the contract addresses to the README in deployments/? Also, will you follow up with deployments of the verifier/multicall handler?

Signed-off-by: james-a-morris <[email protected]>
* feat: deploy ink multicallhandler
---------

Signed-off-by: james-a-morris <[email protected]>
@james-a-morris james-a-morris requested review from pxrl and bmzig December 19, 2024 22:22
Copy link
Contributor

@bmzig bmzig left a comment

Choose a reason for hiding this comment

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

This looks good to me. Are we not able to deploy the spoke pool verifier on this chain? Are we not doing it because the current version is unaudited?

@james-a-morris
Copy link
Contributor Author

This looks good to me. Are we not able to deploy the spoke pool verifier on this chain? Are we not doing it because the current version is unaudited?

Yep but also because this is a low prio contract to deploy and this will begin to block Alex/myself on the rest of the deployments

Signed-off-by: james-a-morris <[email protected]>
@pxrl
Copy link
Contributor

pxrl commented Dec 19, 2024

This looks good to me. Are we not able to deploy the spoke pool verifier on this chain? Are we not doing it because the current version is unaudited?

Yep but also because this is a low prio contract to deploy and this will begin to block Alex/myself on the rest of the deployments

I looked at the deployment process and found it's more convoluted now due to a merge conflict. I was able to make a deployment here: https://explorer.inkonchain.com/address/0xB4A8d45647445EA9FC3E1058096142390683dBC2?tab=contract

Here's a PR to add it: #814

@james-a-morris james-a-morris merged commit 4bb8383 into master Dec 20, 2024
9 checks passed
@james-a-morris james-a-morris deleted the james/acx-3455-deploy-contracts branch December 20, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants