-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add wrapper functionality & bridge integration #281
Conversation
…ent-wrapper-functionality
struct RoyaltyInfo { | ||
address royaltyAddress; | ||
uint96 royaltyBps; | ||
} | ||
|
||
/** | ||
* @dev Initializes the contract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added contract init params: underlyingNftContractAddress
, vmBridgeAddress
, baseTokenURI_
@@ -73,31 +117,33 @@ contract BridgedTopShotMoments is | |||
return _customSymbol; | |||
} | |||
|
|||
function safeMint(address to, uint256 tokenId, string memory uri) public onlyOwner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed safeMint
- minting will only be possible as part of:
- Wrapper functionality
- Bridge fulfillment (when contract is onboarded to upgraded bridge)
|
||
import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; | ||
|
||
interface IBridgePermissions is IERC165 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From flow-evm-bridge repo
@@ -0,0 +1,6 @@ | |||
pragma solidity 0.8.24; | |||
|
|||
interface ICrossVMBridgeFulfillment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interface conformance expected to be needed to onboard a custom contract in upcoming upgraded bridge
* Note: The Flow VM bridge checks for permissions at asset onboarding. If your asset has already | ||
* been onboarded, setting `permissions` to `false` will not affect movement between VMs. | ||
*/ | ||
abstract contract BridgePermissionsUpgradeable is Initializable, ERC165Upgradeable, IBridgePermissions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From flow-evm-bridge repo
* @dev A base contract intended for use in implementations on Flow, allowing a contract to define | ||
* access to the Cadence X EVM bridge on certain methods. | ||
*/ | ||
abstract contract CrossVMBridgeCallableUpgradeable is ContextUpgradeable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapted for upgradable contracts from flow-evm-repo draft PR - the functionality is what's currently expected to register a custom contract in upcoming upgraded bridge
import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; | ||
import {ICrossVMBridgeFulfillment} from "../interfaces/ICrossVMBridgeFulfillment.sol"; | ||
|
||
abstract contract CrossVMBridgeFulfillmentUpgradeable is CrossVMBridgeCallableUpgradeable, ERC165Upgradeable, ERC721Upgradeable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapted for upgradable contracts from flow-evm-repo draft PR - the functionality is what's currently expected to register a custom contract in upcoming upgraded bridge
contracts/TopShot.cdc
Outdated
} | ||
return nil | ||
} | ||
|
||
// resolveCrossVMPointerView resolves the CrossVMPointer view | ||
// access(all) view fun resolveCrossVMPointerView(): MetadataViews.CrossVMPointer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out until this PR's changes are merged and pushed on-chain - this new metadata view should be returned both from contract and nft metadata views; this should provide the custom contract FlowEVM address
import "FlowEVMBridgeConfig" | ||
import "FlowEVMBridgeUtils" | ||
|
||
/// Bridges NFTs with provided IDs from Cadence to EVM and wraps them in a wrapper ERC721 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All-in-one Cadence tx to bridge NFTs from Cadence to EVM and make EVM contract calls to approve wrapper erc721 contract as operator and wrap the bridged NFTs to wrapper erc721 NFTs
Adapted from flow-evm-bridge
's batch_bridge_nft_to_evm.cdc
/// @param nftIDs: Array of IDs of the NFTs to wrap | ||
/// | ||
transaction( | ||
wrapperERC721Address: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter should not be needed anymore once we can get the contract address from new metadata view on-chain
Key Updates:
|
return decodedResult[0] as! EVM.EVMAddress | ||
} | ||
|
||
/// Calls a function on an EVM contract from provided coa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we decided not to deploy an util contract for the mustCall
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it would be best achieved by holding the contract in a trusted neutral entity like the bridge account or a keyless account given the privileged access to COA, but it doesn't seem really worth it at this point as the wrapping/unwrapping logic is needed only temporarily until the bridge gains support for custom associations and unwrapping logic within its own context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! left one question
Changes:
ERC721Wrapper
OpenZeppelin libraryERC721URIStorage
extension (per-token metadata storage isn't currently needed) and usebaseTokenURI
contract state variable instead to build metadata endpoint url fromtokenURI
with token IDBridgePermissionsUpgradeable
to prevent bridge onboarding before bridge upgradeCrossVMBridgeFulfillmentUpgradeable
to allow upgraded bridge to fulfill to EVM (i.e., mint or transfer)