Add NFT bridge interface #19
Conversation
sisyphusSmiling
force-pushed
the
add-bridge-interface
branch
2 times, most recently
from
4755a6c
to
516ecc6
Compare
sisyphusSmiling
force-pushed
the
add-bridge-interface
branch
from
516ecc6
to
7c5ad95
Compare
sisyphusSmiling
force-pushed
the
add-bridge-interface
branch
from
7c5ad95
to
f8c0346
Compare
There was a problem hiding this comment.
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.
Should we also have the bridgeContractName field here?
There was a problem hiding this 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?
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.
| fun bridgeNFTToEVM( | ||
| token: @{NonFungibleToken.NFT}, | ||
| to: EVM.EVMAddress, | ||
| feeProvider: auth(FungibleToken.Withdraw) &{FungibleToken.Provider} |
There was a problem hiding this comment.
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.
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.
Same comments about pre-conditions here
There was a problem hiding this comment.
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.
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.
Ah good catch. I'll clean up params upstream to prevent merge conflicts in the stacked PRs.
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 inEVMBridgeRouterwhich direct calls fromEVM.CadenceOwnedAccounts to the named bridge contract.For contributor use:
masterbranchFiles changedin the Github PR explorer