FEATURE: V2 FungibleToken Standard

[joshuahannan]

This is a different PR from the original one because the git organization had gotten out of sync

Adds the v2 fungible token standard, as described in this flip: onflow/flips#55

A few highlights:

• Turns FungibleToken into a contract instead of a contract interface
• Introduces a new contract interface with a totalSupply field and function to get the vault types that the contract implements. This will help external parties learn about which vaults a contract implements since contracts can implement multiple vault types now
• Adds a Transferable interface to include a transfer function
• Moves standard paths to inside the vault object definition because they are specific to the actual vault instead of the contract now.
• Moves createEmptyVault into the vault resource
• Adds type parameters to the all the events since contracts can have multiple vault types defined

There are probably more that I am missing, but most of the details are in the forum post, so that is another good resource.

[bjartek]

Would it be possible at all for a Vault in FT to optionally contain the address it was withdrawn from? Just beeing able to look at that to know who is paying for things would make some things way easier.

[bluesign]

Would it be possible at all for a Vault in FT to optionally contain the address it was withdrawn from? Just beeing able to look at that to know who is paying for things would make some things way easier.

I think this can be very good case for attachments. Extra metadata like this can be added as an attachment maybe.

[joshuahannan]

I just pushed changes to the standard based on the discussions we've been having in the standard breakout sessions, including events emitted from interfaces.

Would it be possible at all for a Vault in FT to optionally contain the address it was withdrawn from? Just being able to look at that to know who is paying for things would make some things way easier.

I agree this could be good for attachments. It would be possible to do this in a way with the standard, but idk if it is safe

[bjartek]

I agree this could be good for attachments. It would be possible to do this in a way with the standard, but idk if it is safe

Can we get some eyes on this to ensure if it is safe or not?

[joshuahannan]

It is kind of similar to the msg.sender problem. If we had a field on every vault that was the previous owner, it would be easy to spoof that and just give it a different previous owner by depositing it into a different vault. Also, we still can't add fields to resources in upgrades, so I'm not sure how we would add that in an upgrade. Does that make sense?

[sisyphusSmiling]

🚀

[joshuahannan]

@bjartek proposed that we add a balanceAfter field to the FungibleToken.Deposited event to show the balance of the Vault after the deposit happens. I think this is a no-brainer, so unless anyone has any objections, I am going to include it in the standard so that it is in the next emulator/CLI release for Cadence 1.0

Unfortunately, we can't do this for the Withdrawn event because it is emitted from the Provider interface, so it doesn't have access to the balance field

[bjartek]

Does anybody remember why we emit Withdrawn events from the Provider and Deposited from the Vault? Because it would be very very nice to have this for withdraw events too.

[joshuahannan]

After thinking about it a little bit, I am pretty sure that the reason that the Withdrawn emission is in the Provider is because we were just copying what we did for NFTs. Since NFTs can be stored anywhere, it made sense to put the event in the provider because they might not necessarily be stored in a collection. It is different with FTs though since they will always end up in a Vault, so I think it makes sense to move the Withdrawn event emission to Vault and include a balanceAfter field.

[joshuahannan]

I opened a PR for this in the FT repo: #158

Looking for reviews! :)

[dryruner]

There're 2 more questions open for discussion based on recent experiments:

• (1) Current standards doesn't support implemention of transfer-tax tokens in contract level;
• (2) Current standards doesn't support implemention of rebase tokens in contract level;

For (1):

• This type of token is not so rare in erc20 world: basically it charges a tax% on each transfer, for example by transferring 100 tokens the recipient only receives 95 tokens, the rest 5 could be either burnt or sent to treasury address. (There could even be a whitelist that accounts in the list are not subject to the transfer tax: e.g. admin, dex pairs, etc.)

• We've tried this idea on testnet: https://testnet.flowdiver.io/contract/A.ffe93b818e74fae6.TaxableToken?tab=deployments
  It turns out that the post condition in either withdraw() or deposit() makes it impossible with current FT standards.

For (2):

• This type of token is also not uncommon in erc20 world, the RebaseToken1.balanceOf(addr) changes over time, for example: stETH .
• Havne't implemented anything to test yet, but given the balance is a value field instead of a function, it basically means it's also impossible right now.

Open questions:

• Do we want to change the current FT standards to onboard these 2 types (and maybe more types, given the constraint could be relaxed) of tokens in Cadence runtime? (Zach asked if we can make some transfer-tax tokens so it brings the experiment and questions)
• If so do we want to do it in the Cadence-1.0 and FT-v2 upgrade?

[bluesign]

There're 2 more questions open for discussion based on recent experiments:
(1) Current standards doesn't support implemention of transfer-tax tokens in contract level;
(2) Current standards doesn't support implemention of rebase tokens in contract level;

Those are really good points ( use cases) @dryruner. Especially with EVM bridging coming, those can have unintended consequences.

Not sure what would be the correct approach here to be honest. Mapping ERC20 to Flow is troublesome in general. Flow having upgradable contracts and different ownership model makes it super complicated.

For example if we remove balance check from Provider, we change the interface behaviour and now everyone after withdraw has to check if balance == withdrawnBalance. There is probably a lot of code out there just trusting the interface restriction.

I would say transfer-tax tokens and rebase tokens maybe need a new standard with a bit more lax with requirements.

[joshuahannan]

Yes, thank you for bringing this up, @dryruner! We actually used to have a getBalance() function in the v2 standard, but replaced it with the field for various reasons. We can't remove the field any more, but we could potentially add the getBalance() function back and include a default implementation of it that projects could override. Would that be enough to satisfy your needs for rebase tokens?

As for the transfer-tax tokens, I am a little conflicted about that. I like having the post-condition there just to make sure that regular implementations are honest, but now that I think about it, removing it wouldn't be a huge problem because it would be pretty obvious if implementations are doing something different without making it clear to users. I think I would be okay with removing it, but I want to see what others think first and I want to spend a little more time thinking about it.

Luckily, both of these changes would not be breaking, so we don't necessarily need to be in a hurry to get it done before Cadence 1.0. I can put them on the agenda for the next smart contract open house to talk about if we don't have any other pressing topics.

[sisyphusSmiling]

For 1) I'm conflicted because this is ultimately very easy to override in Cadence since ultimately the holder of the Vault has full authority on how that Vault is transferred - a holder could use a wrapper to move the Vault to another account in its full denomination and basically treat the balance as one would treat bills.

Ultimately, it's as @bluesign said, translating ERC20 features to Cadence 1-to-1 leads to some odd constructions - feels a bit like putting up a fence those in the know can simply walk around. However, I understand wanting to have parity with emergent features in other ecosystems. My $0.02 would be that even though these wouldn't be breaking changes, the pre-/post-conditions built into the standard could quickly become implicit interfaces that implementers take for granted and as such we should decide on whether to include them sooner rather than later or at least document that consensus may lead to the future removal of these conditions.

[bluesign]

As for the transfer-tax tokens, I am a little conflicted about that. I like having the post-condition there just to make sure that regular implementations are honest, but now that I think about it, removing it wouldn't be a huge problem because it would be pretty obvious if implementations are doing something different without making it clear to users. I think I would be okay with removing it, but I want to see what others think first and I want to spend a little more time thinking about it.

I think removing post conditions from existing interfaces are pretty bad in general, there are codes relying on that protections.

myVault.deposit(from: <- ftProvider.withdraw(amount: amount)) 

for example this code makes 2 assumptions:

• type of the withdrawn will be same as myVault's type
• amount of the withdrawn will be equal to amount

if we change this assumptions, all code relying on them should be updated.

[joshuahannan]

Yeah, after thinking about it some more, I am pretty sure that both of these use cases can be built to be compatible with the existing token standards without requiring any changes, so I would probably recommend exploring that first before we make any of the suggested changes.

[bluesign]

Agreed with @joshuahannan , for 1 ) you can use allowance, you send tax to vault ( which burns or sends to treasury ) and increases you allowance ( you send 5 tokens, your allowance increases 95 etc ) so you can send 95 tokens afterwards. Practically same thing.

for 2) you can make a new interface, let's say DynamicBalance it can have updateBalance/getBalance function which also updates balance.

[joshuahannan]

I think that it is time to merge this to master! If nobody has any issues with me merging this, I'll merge it on Monday once we've tested it with the latest CLI version. I'm going to create a branch that is based off current master so that we still have a record of the cadence 0.42 versions of the contracts too.

[turbolent]

Amazing work everyone! 👏