-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
4755a6c
to
516ecc6
Compare
516ecc6
to
7c5ad95
Compare
7c5ad95
to
f8c0346
Compare
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.
Code looks good! Only left one small comment.
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?
evmContractAddress: FlowEVMBridgeUtils.getEVMAddressAsHexString( | ||
address: self.getAssociatedEVMAddress(with: result.getType()) | ||
?? panic("Could not find EVM Contract address associated with provided NFT") | ||
), bridgeAddress: self.account.address |
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.
Should we also have the bridgeContractName field here?
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.
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.
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.
Great work! Just a few small comments
fun bridgeNFTToEVM( | ||
token: @{NonFungibleToken.NFT}, | ||
to: EVM.EVMAddress, | ||
feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider} |
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.
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?
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.
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
owner: EVM.EVMAddress, | ||
type: Type, | ||
id: UInt256, | ||
feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider}, |
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.
Same comments about pre-conditions here
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.
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
evmID: evmID, | ||
to: FlowEVMBridgeUtils.getEVMAddressAsHexString(address: to), | ||
evmContractAddress: FlowEVMBridgeUtils.getEVMAddressAsHexString(address:associatedAddress) | ||
) | ||
} | ||
|
||
/// Public entrypoint to bridge NFTs from EVM to Cadence |
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.
You have calldata
listed as a parameter here, but I don't see it in the actual function definition.
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.
Ah good catch. I'll clean up params upstream to prevent merge conflicts in the stacked PRs.
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!
Co-authored-by: Joshua Hannan <joshua.hannan@flowfoundation.org>
Description
Introduces a Bridge interface (
IFlowEVMNFTBridge
) which is implemented in the main bridging contract (FlowEVMBridge
). This enables central bridging events from any implementing contract identifying the asset bridged, the bridge address, and the recipient/requestor of the bridge request. This interface is then leveraged inEVMBridgeRouter
which direct calls fromEVM.CadenceOwnedAccount
s to the named bridge contract.For contributor use:
master
branchFiles changed
in the Github PR explorer