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

Discoverability of FT Vault types #69

Merged
merged 4 commits into from Apr 10, 2023

Conversation

satyamakgec
Copy link
Contributor

This adds a FLIP for introducing an additional method in FungibleToken.Receiver interface to retrieve the expected vault type.

@alilloig
Copy link
Contributor

alilloig commented Feb 6, 2023

Looks good so far! Probably it would be a good thing to include also how TokenForwarder contract will implement this, maybe not in the FLIP but in the future PR for updating the FT repo

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Great proposal. I just made some wording changes to be a little more clear. From now on, can you please break up your paragraphs into multiple lines so we can more easily give comments? Thank you!

flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
/// @return Optional list of vault types.
///
pub fun getExpectedVaultTypes() :[Type] {
return [self.getType()]
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this? I think this would just return FungibleToken.Receiver, right? or does it return the actual underlying type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I have done test for this and it is working as expected. It returns the Vault type like Type<@FlowToken.Vault>(), instead of Receiver type.

Copy link
Member

Choose a reason for hiding this comment

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

thats good news. it would be really nice to have this by default so that token implementations don't have to upgrade to support it, but it doesn't really make sense for custom receivers. Is there a way to ensure that types are only included in this array if they are FungibleToken.Vault types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for custom receivers, Sadly there is no way to provide the correct default implementation, Let me do some testing or digging whether it is possible or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I think it is possible to do so.

pub fun getSupportedVaultTypes(): [Type] {
      // Below check is implemented to make sure that run-time type would
      // only get returned when the parent resource conforms with `FungibleToken.Vault` 
      if self.getType().isSubtype(of: Type<@FungibleToken.Vault>()) {
          return [self.getType()]
      } else {
          return []
      }
 }

Copy link
Member

Choose a reason for hiding this comment

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

That is great! Lets get that tested to make sure it works and include it in the proposal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! I have tested this and it is working as expected. Do you want me to start writing code and test in the flow-ft repo or I would do it once this FLIP is get approved?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you can start getting your PR together in the flow-ft repo

flips/20230206-fungible-token-vault-type-discovery.md Outdated Show resolved Hide resolved
@joshuahannan
Copy link
Member

Once this FLIP is completed and upgraded, we'll need to update the swtichboard anyway to include getSupportedVaultTypes but we shouldn't remove getExpectedVaultTypes because we don't want to break any other code unnecessarily.

Comment on lines +11 to +13
The purpose of this proposal is to enable the Fungible Token (FT) `Receiver`s to return a list of vault types that they can accept.
This would enhance the discoverability of FT vault types and reduce the likelihood of unintended failures in the deposit method because the deposited vault is not supported.
This is because Cadence code trying to perform a deposit would now be able to query the acceptable vault types before making a deposit.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The purpose of this proposal is to enable the Fungible Token (FT) `Receiver`s to return a list of vault types that they can accept.
This would enhance the discoverability of FT vault types and reduce the likelihood of unintended failures in the deposit method because the deposited vault is not supported.
This is because Cadence code trying to perform a deposit would now be able to query the acceptable vault types before making a deposit.
The objective of this proposal is to enable Fungible Token (FT) `Receiver`s to return a list of accepted vault types. It is proposed to modify the existing FT standard `Receiver` interface to solve this at a platform level.
This change enhances the discoverability of FT vault types and reduces the likelihood of unintended failures in the deposit method when an unsupported vault is passed in the transaction.
This makes it easy to confirm the accepted vault types for the receiver account before making a deposit.

Comment on lines +20 to +22
Currently, there is no programmatic way to determine the type of token that a `Receiver` is able receive, leading to the possibility of a failed deposit due to an incorrect assumption about the Vault that the `Receiver` is linked to.
To address this issue, a new proxy receiver, the [`FungibleTokenSwitchboard`](https://github.com/onflow/flow-ft/blob/master/contracts/FungibleTokenSwitchboard.cdc), was created to allow for the receipt of multiple Vault types through a single `FungibleToken.Receiver` capability.
The [`SwitchboardPublic`](https://github.com/onflow/flow-ft/blob/4416bbe585629671d00d3acfa6fd8052104dd861/contracts/FungibleTokenSwitchboard.cdc#L37) interface also includes the `getVaultTypes` function, which solves the problem of knowing what vaults the receiver accepts, but only for the switchboard and only for code that is aware of the switchboard.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Currently, there is no programmatic way to determine the type of token that a `Receiver` is able receive, leading to the possibility of a failed deposit due to an incorrect assumption about the Vault that the `Receiver` is linked to.
To address this issue, a new proxy receiver, the [`FungibleTokenSwitchboard`](https://github.com/onflow/flow-ft/blob/master/contracts/FungibleTokenSwitchboard.cdc), was created to allow for the receipt of multiple Vault types through a single `FungibleToken.Receiver` capability.
The [`SwitchboardPublic`](https://github.com/onflow/flow-ft/blob/4416bbe585629671d00d3acfa6fd8052104dd861/contracts/FungibleTokenSwitchboard.cdc#L37) interface also includes the `getVaultTypes` function, which solves the problem of knowing what vaults the receiver accepts, but only for the switchboard and only for code that is aware of the switchboard.
The design of the Fungible Token standard offers no built-in way to determine the token type that a `Receiver` handles. This leaves an unresolved edge case where deposits may fail due to incompatible Vault types being linked to the `Receiver`.
To address this, a new proxy receiver, the [`FungibleTokenSwitchboard`](https://github.com/onflow/flow-ft/blob/master/contracts/FungibleTokenSwitchboard.cdc), was created to handle multiple Vault types through a single `FungibleToken.Receiver` capability.
The [`SwitchboardPublic`](https://github.com/onflow/flow-ft/blob/4416bbe585629671d00d3acfa6fd8052104dd861/contracts/FungibleTokenSwitchboard.cdc#L37) interface provides the `getVaultTypes` function to answer which vaults the receiver accepts, however, this only applies to code in the switchboard.

@joshuahannan
Copy link
Member

@satyamakgec What is the status of this? I'd love to get this finished and upgraded ASAP. Have you been able to work on the implementation in the flow-ft repo?

@franklywatson
Copy link
Contributor

@joshuahannan I checked with Satyam earlier and this is his next priority

pub fun getSupportedVaultTypes() :{Type: Bool} {
// Below check is implemented to make sure that run-time type would
// only get returned when the parent resource conforms with `FungibleToken.Vault`.
if self.getType().isSubtype(of: Type<@FungibleToken.Vault>()) {

Choose a reason for hiding this comment

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

Receiver actually checks the FT type in the pre-condition in L219: https://f.dnz.dev/0x9a0766d93b6608b7/FungibleToken.

Also with the pre-condition check, FT.receiver cannot take multiple types of vaults iirc, which also makes total sense ...

Copy link
Member

Choose a reason for hiding this comment

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

That check is happening in the Vault.deposit method, which makes it work for only Vault implementations. any resource can implement Receiver, and the deposit method in Receiver doesn't care about what kind of Vault it is

@satyamakgec
Copy link
Contributor Author

@joshuahannan can you please approve and merge this as it is already implemented and deployed. Do you think of any place where we need to update the documentation to reflect this change ?

@joshuahannan joshuahannan merged commit a76dc74 into main Apr 10, 2023
@joshuahannan joshuahannan deleted the fungibleToken-standard-improvement branch April 10, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Drafted
Development

Successfully merging this pull request may close these issues.

None yet

5 participants