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

Modularize Solidity contracts for upgradeability & extensibility #67

Merged
merged 21 commits into from
May 9, 2024

Conversation

sisyphusSmiling
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling commented May 6, 2024

Closes: #66

Description

To enhance the extensibility and modularity of the EVM contract, this PR refactors the EVM factory contract into 3 distinct entities:

  1. Deployment registry - maintains state associations between factory-deployed EVM contracts and their corresponding Cadence implementations, e.g. A.123.ExampleNFT.NFT => 0x345...789 and vice versa
  2. EVM contract deployers - tasked with deploying a contract from template using the deploy interface method
  3. Factory - primary entrypoint for the bridge COA which enables the deployment of new EVM-defining contracts for Cadence-native assets and utility methods to inspect EVM state.

Of these three, the registry is intended for permanent use. By decoupling state from functionality, we're able to upgrade to new factory contract version (e.g. add util methods, etc.), add new deployers (e.g. support bridging novel Cadence assets as totally new ERCs or new ERC combinations, etc.).


For contributor use:

  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.03%. Comparing base (428634e) to head (6c4c835).
Report is 110 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
+ Coverage   82.88%   86.03%   +3.15%     
==========================================
  Files          18       18              
  Lines         818      888      +70     
==========================================
+ Hits          678      764      +86     
+ Misses        140      124      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review May 7, 2024 00:27
@sisyphusSmiling sisyphusSmiling requested a review from a team as a code owner May 7, 2024 00:27
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

I mainly looked over the Solidity contracts. The overall organization of the contracts looks good to me. Didn't notice any implementation issues, but suggested a few documentation changes.

cadence/contracts/bridge/FlowEVMBridgeUtils.cdc Outdated Show resolved Hide resolved
solidity/src/FlowBridgeDeploymentRegistry.sol Outdated Show resolved Hide resolved
solidity/src/FlowEVMBridgedERC20Deployer.sol Outdated Show resolved Hide resolved
solidity/src/FlowEVMBridgedERC721Deployer.sol Outdated Show resolved Hide resolved
Copy link

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

Looks good to me, though I have limited skills in reviewing codes in solidity.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

I didn't really review the Solidity code much but everything else looks fine

view fun isValidCadenceAsset(type: Type): Bool {
let isCadenceNFT = type.isSubtype(of: Type<@{NonFungibleToken.NFT}>())
let isCadenceFungibleToken = type.isSubtype(of: Type<@{FungibleToken.Vault}>())
return isCadenceNFT != isCadenceFungibleToken
Copy link
Member

Choose a reason for hiding this comment

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

Technically it is possible for assets to be both fungible and non-fungible tokens, right? Are we not supporting that for simplicity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically it is possible for assets to be both fungible and non-fungible tokens, right?

Not without function overloading in Cadence since both have functions named deposit and `withdraw.

Are we not supporting that for simplicity?

That's right - mutually exclusive implementations are the only use cases supported for v1. That may change in the future. While it's not currently possible to have a mixed FT/NFT due to the need for function overloading, I added the check here anyway in case it does become technically possible at some point in the future.

@sisyphusSmiling sisyphusSmiling merged commit c72a297 into main May 9, 2024
2 checks passed
@sisyphusSmiling sisyphusSmiling deleted the solidity-contract-updates branch May 9, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Deployment Requires contract deployment or update EVM Improvement
Projects
Status: Done
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Enhance EVM upgradeability
5 participants