Add fungible token support #28

sisyphusSmiling · 2024-04-06T00:25:58Z

Description

This PR adds support for fungible token bridging, largely following patterns & interfaces found in the existing NFT bridge feature set.

In addition, some optimizations were made where low-hanging fruit was found. These include

Refactoring ERC identifying fields such as name and symbol with getters. This prevents widely redundant storage at the token-level of fields that are actually contract-level values in ERC contracts.
Adding params to access(account methods (such as initializeEscrow) to reduce redundant EVM calls made in a single bridging transaction

As mentioned in the FLIP, support for non-standard cases will be added, but is considered out of scope for the introduction of the feature and will follow in subsequent PRs.

For contributor use:

Targeted PR against master 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

…es only so far, untested, undeployable atm.

… tests

…arency

…sertion

…es for efficiency

nvdtf · 2024-04-08T17:22:09Z

cadence/contracts/bridge/FlowEVMBridge.cdc

+                symbol: onboardingValues.symbol,
+                erc721Address: onboardingValues.evmContractAddress
+            )
+        } else {


Might want to make the check explicit for fungible tokens and panic if neither NFT or FT

Since that check is done in several places, I include it in typeRequiresOnboarding() which is validated in the pre-condition. Beyond the pre-check, we know the type is either an NFT or FT. By the point of the if block above, we know it's either NFT or FT, and by the else block it must be FT. Do you think I should organize these checks differently?

Makes sense.
I personally like very explicit checks because sometimes assumptions break with future code updates. Not confident it applies here tho

I can see how it would be more resilient to add the explicit checks here. Doesn't hurt to add them so I'll include an else if + else -> panic

cadence/contracts/bridge/FlowEVMBridge.cdc

nvdtf · 2024-04-08T17:35:51Z

cadence/contracts/bridge/FlowEVMBridge.cdc

+            assert(!isERC20, message: "Contract is mixed asset and is not currently supported by the bridge")
+            // Derive the contract name from the ERC721 contract
+            cadenceContractName = FlowEVMBridgeUtils.deriveBridgedNFTContractName(from: evmContractAddress)
+        } else {


Might want an explicit check and panic here too. It stops any errors to fall through the cracks.

Do you mean check isERC20? Further up the call stack, isERC20 would've been called

onboardByEVMAddress |_evmAddressRequiresOnboarding |_FlowEVMBridgeUtils.isValidEVMAsset |_isERC20 ... |_deployDefiningContract

When I was testing locally, I found the redundant EVM calls consumed a lot of gas and made a pass at minimizing them.. But if we feel the explicit checks are worth it I can add them back in.

cadence/contracts/bridge/FlowEVMBridge.cdc

joshuahannan · 2024-04-10T21:02:37Z

cadence/contracts/bridge/FlowEVMBridge.cdc

@@ -74,14 +109,37 @@ contract FlowEVMBridge : IFlowEVMNFTBridge {
        let feeVault <-feeProvider.withdraw(amount: FlowEVMBridgeConfig.onboardFee) as! @FlowToken.Vault
        FlowEVMBridgeUtils.deposit(<-feeVault)
        // Deploy an EVM defining contract via the FlowBridgeFactory.sol contract
-        let erc721Address = self.deployEVMContract(forAssetType: type)
+        // let evmContractAddress = self.deployEVMContract(forAssetType: type)


Can this be removed since it is commented out?

Ah yeah, thanks for catching that

joshuahannan · 2024-04-10T21:32:55Z

cadence/contracts/bridge/FlowEVMBridgeNFTEscrow.cdc

        /// Maps EVM NFT ID to Flow NFT ID, covering cross-VM project NFTs
-        access(self)
-        let evmIDToFlowID: {UInt256: UInt64}
+        access(self) let evmIDToFlowID: {UInt256: UInt64}


Would it make sense to have a reverse mapping of Cadence ID to EVM ID?

Yeah that could make sense. To minimize the storage usage, I check against the EVMNFT implementation on demand in getEVMID. It's the same mechanism I would use to assign a value in a reverse mapping but without the associated storage cost. Think it's worth the cost here?

joshuahannan · 2024-04-10T21:43:22Z

cadence/contracts/bridge/FlowEVMBridgeTokenEscrow.cdc

+    /// Initializes the Locker for the given fungible token type if it hasn't been initialized yet
+    ///
+    access(account) fun initializeEscrow(
+        with vault: @{FungibleToken.Vault},


what is the with here?

with vault: @{FungibleToken.Vault} is the FT vault being used to initialize escrow. FT Escrow needs an empty vault to wrap in a Locker so that future bridged funds can be escrowed. Think the parameter naming is alright?

joshuahannan · 2024-04-10T22:43:26Z

cadence/contracts/test/EVMDeployer.cdc

-        self.deployedAddress = coa.deploy(
+    access(all) let deployedAddresses: {String: EVM.EVMAddress}
+
+    access(all) fun deploy(name: String, bytecode: String, value: UInt) {


Should this be access(all) or something more restrictive?

This contract is a temporary test measure until I can get visibility into EVM events from within the testing framework. The problem is I need to know the EVM address of a deployed contract and can only discover that by saving the address somewhere upon deployment.

Wrapping in a resource would be the more secure practice here, but I made it a public method to reduce complexity in testing. Just added a contract-level comment at the top, lmk if you think I should move this to a resource.

sisyphusSmiling · 2024-04-11T00:33:12Z

Thanks for the review @joshuahannan! I pushed changes based on recent feedback. Those are largely in two commits:

96e9226 adds updated access, restricting asset bridging from EVM to the COA interface methods
83d7c5f adds $FLOW handling for requests bridging FlowToken to EVM

aishairzay · 2024-04-11T20:39:01Z

cadence/contracts/bridge/FlowEVMBridge.cdc

+        let cadenceAddress = FlowEVMBridgeUtils.getContractAddress(fromType: forTokenType)
+            ?? panic("Could not derive contract address for token type: ".concat(identifier))
+        // Assign a default symbol
+        var symbol = "BRDG"


Rather than having a default, I think it might be better to panic if neither metadata field is filled in as expected.

I don't think anyone would want to accidentally have their token bridged with a name/ticker that can't be changed on all evm consuming indexed websites which they didnt have control over

Yeah, that's a good point. One of those calls that's a balance between ensuring bridging is permissionless with respecting how projects wish to be represented tough. That said, I'd be surprised if a Cadence NFT/FT doesn't define this value so I agree, it's probably best to just revert here

Looking at MetadataViews, there isn't a view that supports symbol. So we can make this call for fungible tokens, but we'll have to consider how to dynamically create a symbol if a project doesn't implement the EVMBridgedMetadata view. Maybe the first n characters of the name with special characters and spaces removed? Have any thoughts?

Update: Created #29 to track discussion

cadence/contracts/bridge/IEVMBridgeTokenMinter.cdc

Co-authored-by: Navid TehraniFar <[email protected]>

aishairzay and others added 30 commits April 3, 2024 15:57

Begin adding support for fungible tokens in the bridge. Cadence chang…

703fc25

…es only so far, untested, undeployable atm.

update CrossVMFT to CrossVMToken

d33afc4

rename FlowEVMBridgeFTEscrow to FlowEVMBridgeTokenEscrow

53f94ce

add FT util to FlowEVMBridgeUtils

e2f6c7b

rename IEVMBridgeFTMinter to IEVMBridgeTokenMinter

ffdecb2

update bridge with new token escrow interface

afb49da

fix FlowEVMBridgeUtils deployment error

a97985d

update FlowBridgeFactory to validate ERC20 implementation

74da8ee

add FlowBridgedERC20 & add deployment + ERC20 inspection to Factory +…

582e754

… tests

integrate ERC20 factory methods into bridge contracts

7535586

add FTDisplay serialization to SerializeNFT

b922c91

add deployERC20 internal method to bridge contract

040d2a3

fix token escrow syntax errors

af1166b

add balance inspection utils

39c14a0

add fungible token support to FlowEVMBridge

63c2e7c

add FLOW exemption and fix IFlowEVMTokenBridge

463a0a7

integrate fungible token bridging into EVM contract

1a38948

fix contract deployment errors for passing test setup

9c25262

update ExampleToken

1679781

update EVM contract's update bytecode hex in tests

22a88fc

update CrossVMToken.EVMFTVault interface

b6e3e0d

add test status & coverage badges to README

c758f4c

update Minter interfaces to emit identifying events for better transp…

09e2b54

…arency

generalize onboardByType for NFT & FT onboarding & add escrow init as…

09f89df

…sertion

update token escrow Locker for CrossVMToken.EVMFTVault conformance

13feb26

update comments

38c0c73

update CrossVMNFT interfaces + implementations & escrow init interfac…

178783e

…es for efficiency

update CrossVMToken interface & implementations

3c37e10

fix Cadence contract FT deployment in FlowEVMBridge contract

69ba69a

add test support for FT bridging

51678ac

nvdtf reviewed Apr 8, 2024

View reviewed changes

sisyphusSmiling added 2 commits April 10, 2024 15:41

Update EVM BridgeAccessor entitlements

0259c8f

add explicit type check in onboardByType

2c7c9e0

sisyphusSmiling commented Apr 10, 2024

View reviewed changes

cadence/contracts/bridge/FlowEVMBridge.cdc Outdated Show resolved Hide resolved

joshuahannan approved these changes Apr 10, 2024

View reviewed changes

sisyphusSmiling added 5 commits April 10, 2024 18:49

Update bridge from EVM to access(account) & integrate w/ EVM contract

96e9226

update comment in EVMDeployer test contract

3cc726b

remove FlowEVMBridge comment

a692e35

update EVMAddress interface based on new standard lib

587ed52

enable FLOW bridging to EVM via EVMAddress passthrough

83d7c5f

sisyphusSmiling requested a review from joshuahannan April 11, 2024 00:37

aishairzay reviewed Apr 11, 2024

View reviewed changes

update .gitignore

11dcd70

aishairzay reviewed Apr 12, 2024

View reviewed changes

cadence/contracts/bridge/IEVMBridgeTokenMinter.cdc Outdated Show resolved Hide resolved

fix EVM contract imports

a6cc0fc

This was referenced Apr 12, 2024

[EVM] Add 1.0 bridging interface to COA onflow/flow-go#5664

Closed

Dynamically assign an ERC721 symbol for bridged NFTs #29

Closed

sisyphusSmiling added 2 commits April 12, 2024 17:17

update onboarding logic to prevent FT w/ unspecified symbol

2519c65

add updated EVM contract methods

d877f20

sisyphusSmiling mentioned this pull request Apr 12, 2024

Rename SerializeNFT to SerializeMetadata #30

Closed

sisyphusSmiling added 2 commits April 12, 2024 19:44

add assertion on non-nil symbol value in ERC20 deployment

4d45b10

fix tests by updating EVM contract in lates CLI release

f60b5f1

sisyphusSmiling mentioned this pull request Apr 13, 2024

Add fees #32

Merged

6 tasks

sisyphusSmiling and others added 2 commits April 15, 2024 15:46

Apply suggestions from code review

e6b186c

Co-authored-by: Navid TehraniFar <[email protected]>

add FLOW filter on fromEVM requests & remove unused entitlement

e13f8a9

joshuahannan approved these changes Apr 15, 2024

View reviewed changes

fix failing Cadence tests

c1cc1aa

sisyphusSmiling merged commit 9b6dc21 into main Apr 15, 2024
2 checks passed

sisyphusSmiling deleted the add-ft-bridge branch April 15, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fungible token support #28

Add fungible token support #28

sisyphusSmiling commented Apr 6, 2024 •

edited

Loading

nvdtf Apr 8, 2024

sisyphusSmiling Apr 9, 2024

nvdtf Apr 10, 2024

sisyphusSmiling Apr 10, 2024

nvdtf Apr 8, 2024

sisyphusSmiling Apr 9, 2024 •

edited

Loading

joshuahannan Apr 10, 2024

sisyphusSmiling Apr 10, 2024

joshuahannan Apr 10, 2024

sisyphusSmiling Apr 10, 2024

joshuahannan Apr 10, 2024

sisyphusSmiling Apr 12, 2024

joshuahannan Apr 10, 2024

sisyphusSmiling Apr 10, 2024

sisyphusSmiling commented Apr 11, 2024

aishairzay Apr 11, 2024

sisyphusSmiling Apr 12, 2024

sisyphusSmiling Apr 12, 2024 •

edited

Loading

Add fungible token support #28

Add fungible token support #28

Conversation

sisyphusSmiling commented Apr 6, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sisyphusSmiling Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

sisyphusSmiling commented Apr 6, 2024 •

edited

Loading

sisyphusSmiling Apr 9, 2024 •

edited

Loading

sisyphusSmiling Apr 12, 2024 •

edited

Loading