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

Add NFT bridge interface #19

Merged
merged 6 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add IFlowEVMNFTBridge contract interface & implement in FlowEVMBridge
  • Loading branch information
sisyphusSmiling committed Mar 26, 2024
commit a2a3d8310ef6424d197dab42035906d404dc0445
10 changes: 9 additions & 1 deletion cadence/contracts/bridge/FlowEVMBridge.cdc
Copy link
Member

Choose a reason for hiding this comment

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

I think that in some of the functions when you cast to a FlowToken.Vault, if it isn't FlowToken, then you'll just get an Error that isn't helpful. Might be good to have a better error message that says that the provider is the incorrect type

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import "EVM"
import "BridgePermissions"
import "ICrossVM"
import "IEVMBridgeNFTMinter"
import "IFlowEVMNFTBridge"
import "CrossVMNFT"
import "FlowEVMBridgeConfig"
import "FlowEVMBridgeUtils"
Expand All @@ -25,7 +26,7 @@ import "FlowEVMBridgeTemplates"
/// - FLIP #237: https://github.com/onflow/flips/pull/233
///
access(all)
contract FlowEVMBridge {
contract FlowEVMBridge : IFlowEVMNFTBridge {

/*************
Events
Expand Down Expand Up @@ -323,6 +324,13 @@ contract FlowEVMBridge {
Public Getters
**************************/

/// Returns the EVM address associated with the provided type
///
access(all)
view fun getAssociatedEVMAddress(with type: Type): EVM.EVMAddress? {
return FlowEVMBridgeConfig.getEVMAddressAssociated(with: type)
}

/// Retrieves the bridge contract's COA EVMAddress
///
/// @returns The EVMAddress of the bridge contract's COA orchestrating actions in FlowEVM
Expand Down
109 changes: 109 additions & 0 deletions cadence/contracts/bridge/IFlowEVMNFTBridge.cdc
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import "FungibleToken"
import "NonFungibleToken"

import "EVM"

import "FlowEVMBridgeConfig"
import "FlowEVMBridgeUtils"

access(all) contract interface IFlowEVMNFTBridge {

/*************
Events
**************/

/// Broadcasts an NFT was bridged from Cadence to EVM
access(all)
event BridgedNFTToEVM(
type: Type,
id: UInt64,
evmID: UInt256,
to: String,
evmContractAddress: String,
bridgeAddress: Address
)
/// Broadcasts an NFT was bridged from EVM to Cadence
access(all)
event BridgedNFTFromEVM(
type: Type,
id: UInt64,
evmID: UInt256,
caller: String,
evmContractAddress: String,
bridgeAddress: Address
)

/**************
Getters
***************/

/// Returns the EVM address associated with the provided type
///
access(all)
view fun getAssociatedEVMAddress(with type: Type): EVM.EVMAddress?

/********************************
Public Bridge Entrypoints
*********************************/

/// Public entrypoint to bridge NFTs from Cadence to EVM.
///
/// @param token: The NFT to be bridged
/// @param to: The NFT recipient in FlowEVM
/// @param feeProvider: A reference to a FungibleToken Provider from which the bridging fee is withdrawn in $FLOW
///
access(all)
fun bridgeNFTToEVM(
token: @{NonFungibleToken.NFT},
to: EVM.EVMAddress,
feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider}
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to put a pre-condition in these to make sure the Provider has sufficient balance? And it has to be a FlowToken provider, right? Maybe we can have a pre-condition to validate that also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The difficulty is that when bridging an NFT to EVM, the NFT fee will be dynamically calculated based on the storage it consumes. Calculating that fee requires saving the NFT which is not a view method and so couldn't be added to a pre-condition

) {
pre {
emit BridgedNFTToEVM(
type: token.getType(),
id: token.id,
evmID: 0,
to: FlowEVMBridgeUtils.getEVMAddressAsHexString(address: to),
evmContractAddress: FlowEVMBridgeUtils.getEVMAddressAsHexString(
address: self.getAssociatedEVMAddress(with: token.getType())
?? panic("Could not find EVM Contract address associated with provided NFT")
), bridgeAddress: self.account.address
)
}
}

/// Public entrypoint to bridge NFTs from EVM to Cadence
///
/// @param owner: The EVM address of the NFT owner. Current ownership and successful transfer (via
/// `protectedTransferCall`) is validated before the bridge request is executed.
/// @param calldata: Caller-provided approve() call, enabling contract COA to operate on NFT in EVM contract
/// @param id: The NFT ID to bridged
/// @param evmContractAddress: Address of the EVM address defining the NFT being bridged - also call target
/// @param feeProvider: A reference to a FungibleToken Provider from which the bridging fee is withdrawn in $FLOW
/// @param protectedTransferCall: A function that executes the transfer of the NFT from the named owner to the
/// bridge's COA. This function is expected to return a Result indicating the status of the transfer call.
///
/// @returns The bridged NFT
///
access(all)
fun bridgeNFTFromEVM(
owner: EVM.EVMAddress,
type: Type,
id: UInt256,
feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider},
Copy link
Member

Choose a reason for hiding this comment

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

Same comments about pre-conditions here

protectedTransferCall: fun (): EVM.Result
): @{NonFungibleToken.NFT} {
post {
emit BridgedNFTFromEVM(
type: result.getType(),
id: result.id,
evmID: id,
caller: FlowEVMBridgeUtils.getEVMAddressAsHexString(address: owner),
evmContractAddress: FlowEVMBridgeUtils.getEVMAddressAsHexString(
address: self.getAssociatedEVMAddress(with: result.getType())
?? panic("Could not find EVM Contract address associated with provided NFT")
), bridgeAddress: self.account.address
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also have the bridgeContractName field here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For simplicity purposes of the EVM bridge as a whole though, are we sure we want to allow other bridges to emit these exact same events?

Is there a use-case inspiring this, or is it just to allow for future similar use-cases? Are there some risks we're introducing from this, or is the guidance that consumers can just filter based on contract address and its all good?

It's a good question. I don't think we'd need to worry about other contracts impersonating the central bridge since listeners could filter on bridgeAddress which the interface guarantees will be accurate. Curious if anyone can think of vulnerabilities this might introduce.

Regarding use case - I thought it would be helpful at minimum which prototyping the bridge on Previewnet. In the event we need to deploy updates to a new account, listeners could just filter on the updated address. Ideally, we'd get visibility into all assets bridging between VMs with this interface, but the reversion on evmContractAddress likely limits the usefulness of this interface to just this public infra bridge.

Should we also have the bridgeContractName field here?

I'm not sure there's a way to dynamically retrieve the contract name from within the contract at runtime 🤔 But I could see it being helpful.

)
}
}
}