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

Fungible Token Core Standard #141

Closed
evgenykuzyakov opened this issue Jan 8, 2021 · 66 comments
Closed

Fungible Token Core Standard #141

evgenykuzyakov opened this issue Jan 8, 2021 · 66 comments

Comments

@evgenykuzyakov
Copy link
Contributor

evgenykuzyakov commented Jan 8, 2021

Fungible Token Core Standard

Background

This standard is a subset of the proposed #136 that only covers the core contract API.
Please read the linked standard.

It doesn't include token metadata standard that will be covered by a separate NEP, because the metadata may evolve.
It also doesn't include account registration standard that also should be covered by a separate NEP because it can be reused for other contract.

It's also not exact core API from the #136
This standard allows to specify the amount of tokens used by the contract and refund a part. It also renames some arguments to
be more consistent with the original fungible token standard #21

See #122 (comment) for the rationale to move from #122

Proposal

API

Token contract should implement the following Trait.

/// Core functionality of a Fungible Token contract
pub trait FungibleTokenCore {
    /// Simple transfer.
    /// Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id`.
    /// Both accounts should be registered with the contract for transfer to succeed.
    /// Method is required to be able to accept attached deposits - to not panic on attached deposit. See Security
    /// section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    #[payable]
    fn ft_transfer(
        &mut self,
        receiver_id: ValidAccountId,
        amount: U128,
        memo: Option<String>
    );

    /// Transfer to a contract with a callback.
    /// Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id` account. Then
    /// calls `ft_on_transfer` method on `receiver_id` contract and attaches a callback to resolve this transfer.
    /// `ft_on_transfer` method should return the amount of tokens used by the receiver contract, the remaining tokens
    /// should be refunded to the `predecessor_account_id` at the resolve transfer callback.
    ///
    /// Token contract should pass all the remaining unused gas to the `ft_on_transfer` call.
    ///
    /// Malicious or invalid behavior by the receiver's contract:
    /// - If the receiver contract promise fails or returns invalid value, the full transfer amount should be refunded.
    /// - If the receiver contract overspent the tokens, and the `receiver_id` balance is lower than the required refund
    /// amount, the remaining balance should be refunded. See Security section of the standard.
    ///
    /// Both accounts should be registered with the contract for transfer to succeed.
    /// Method is required to be able to accept attached deposits - to not panic on attached deposit. See Security
    /// section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver contract. This contract will be called.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `msg` - a string message that will be passed to `ft_on_transfer` contract call.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    /// Returns a promise to resolve transfer call which will return the used amount (see suggested trait to resolve
    /// transfer).
    #[payable]
    fn ft_transfer_call(
        &mut self,
        receiver_id: ValidAccountId,
        amount: U128,
        msg: String,
        memo: Option<String>,
    ) -> Promise;

    //////////////////
    // View methods //
    //////////////////

    /// Returns the total supply of the token.
    fn ft_total_supply(&self) -> U128;

    /// Returns the balance of the given account ID. Returns `0` balance, if the account doesn't exist.
    fn ft_balance_of(&self, account_id: ValidAccountId) -> U128;
}

The receiver contract should implement the following Trait

/// Receiver of the Fungible Token for `transfer_call` calls.
pub trait FungibleTokenReceiver {
    /// Callback to receive tokens.
    /// Called by fungible token contract `env::predecessor_account_id` after `transfer_call` was initiated by
    /// `sender_id` of the given `amount` with the transfer message given in `msg` field.
    /// The `amount` of tokens were already transferred to this contract account and ready to be used.
    ///
    /// The method should return the amount of tokens that are used/accepted by this contract from the transferred
    /// amount. Examples:
    /// - The transferred amount was `500`, the contract completely takes it and should return `500`.
    /// - The transferred amount was `500`, but this transfer call only needs `450` for the action passed in the `msg`
    ///   field, then the method should return `450`.
    /// - The transferred amount was `500`, but the action in `msg` field has expired and the transfer should be
    ///   cancelled. The method should return `0` or panic.
    ///
    /// Arguments:
    /// - `sender_id` - the account ID that initiated the transfer.
    /// - `amount` - the amount of tokens that were transferred to this account.
    /// - `msg` - a string message that was passed with this transfer call.
    /// Returns the amount of tokens that are used/accepted by this contract from the transferred amount.
    fn ft_on_transfer(
        &mut self,
        sender_id: ValidAccountId,
        amount: U128,
        msg: String,
    ) -> PromiseOrValue<U128>;
}

Suggested Trait to handle the callback on fungible token contract to resolve transfer. It's not a public interface, so
fungible token contract can implement it differently.

pub trait FungibleTokenCoreResolveTransfer {
    /// Callback to resolve transfer.
    /// Private method (`env::predecessor_account_id == env::current_account_id`).
    ///
    /// Called after the receiver handles the transfer call and returns value of used amount in `U128`.
    ///
    /// This method should get `used_amount` from the receiver's promise result and refund the remaining
    /// `amount - used_amount` from the receiver's account back to the `sender_id` account. Methods returns the amount
    /// tokens that were spent from `sender_id` after the refund (`amount - min(receiver_balance, used_amount)`)
    ///
    /// Arguments:
    /// - `sender_id` - the account ID that initiated the transfer.
    /// - `receiver_id` - the account ID of the receiver contract.
    /// - `amount` - the amount of tokens that were transferred to receiver's account.
    /// Promise results:
    /// - `used_amount` - the amount of tokens that were used by receiver's contract. Received from `on_ft_receive`.
    ///   `used_amount` should be `U128` in range from `0` to `amount`. All other invalid values are considered to be
    ///   equal to `0`.
    /// Returns the amount of tokens that were spent from the `sender_id` account. Note, this value might be different
    /// from the `used_amount` returned by the receiver contract, in case the refunded balance is not available on the
    /// receiver's account.
    #[private]
    fn resolve_transfer(
        &mut self,
        sender_id: ValidAccountId,
        receiver_id: ValidAccountId,
        amount: U128,
        #[callback_result] used_amount: CallbackResult<U128>,  // NOTE: this interface is not supported yet and has to
                                                               // be handled using lower level interface.
    ) -> U128;
}

Notes

memo field is explained in details in #136
The goal is to associate some external information with the transfer on chain. It may be needed for compliance in some
jurisdictions.

Security

Requirement for accept attached deposits (#[payable])

Due to the nature of function-call permission access keys on NEAR protocol, the method that requires an attached deposit
can't be called by the restricted access key. If the token contract requires an attached deposit of at least 1
yoctoNEAR on transfer methods, then the function-call restricted access key will not be able to call them without going
through the wallet confirmation. This prevents some attacks like fishing through an authorization to a token contract.

This 1 yoctoNEAR is not enforced by this standard, but is encouraged to do. While ability to receive attached deposit
is enforced by this token.

Transfer Call Refunds

If the receiver contract is malicious or incorrectly implemented, then the receiver's promise result may be invalid and
the required balance may not be available on the receiver's account. In this case the refund can't be provided provided
to the sender. This is prevented by #122 standard that locks funds into a temporary
vault and prevents receiver from overspending the funds and later retuning invalid value.
But if this flaw exist in this standard, it's not an issue for the sender account. It only affects the transfer amount
and the receiver's account balance. The receiver can't overspend tokens from the sender outside of sent amount, so this
standard should be considered as safe as #122

@oysterpack
Copy link

oysterpack commented Jan 8, 2021

first off - thanks @evgenykuzyakov for kicking this off

I propose we take the following iterative approach to keep the online discussion manageable and progressing towards the end goal

  1. Let's start with the above interface as the baseline starting point.
  2. Propose a change by starting your comment title with: Change Proposal: <TITLE>.
  3. Community can then discuss and focus on a specific Change Proposal.
    • when commenting on a change proposal, reference the Change Proposal: <TITLE>
    • community members can express their acceptance or rejection of the proposal via a thumbs up/down reaction
    • if you vote thumbs down, please explain why
  4. Once there appears to be general consensus (measured by thumbs up/down reactions), then propose an official vote on the Change Proposal. The comment should be titled with Change Proposal Vote: <TITLE>. The comment should re-specify the full spec in the comment with the proposed change and reference the original Change Proposal via a link
    • if there appears to be overall rejection of the proposal, then close it out by renaming the title to REJECTED: Change Proposal: <TITLE>
  5. We set a deadline for community members to vote via a thumbs up or thumbs down. We can start with a deadline of 2 days to collect votes.
    • After the voting deadline, update the comment that publishes the proposed change voting outcome, i.e., PASSED: Change Proposal or REJECTED: Change Proposal
    • if the proposed change passes, then the spec is re-baselined with the proposed change.
    • if a community member votes thumbs down, it would be good to post a comment explaining why - perhaps this could influence others to change their vote before the voting deadline - and conversely

Deliverable Goals

The end product should be a Rust library module published on https://crates.io/.
The project repository location is TBD - perhaps it should live within https://github.com/near

Thoughts?

I will be submitting my first proposed change shortly ...

@oysterpack
Copy link

oysterpack commented Jan 8, 2021

ACCEPTED: Change Proposal: namespace interface function names ft_ prefix

  • The purpose of the change is to provide a namespace for the interface functions on the contract.
  • This will help avoid naming collisions with other contract functions and interfaces.
    • For example, for NFTs, the interface would be similar. And if a contract chose to implement both an NFT and FT token
      then this would ensure there are no function collisions for the transfer function, e.g., ft_transfer vs nft_transfer

Proposed Interface Changes

  • rename the following functions on the FungibleTokenCore
    • transfer -> ft_transfer
    • transfer_call -> ft_transfer_call
    • total_supply -> ft_total_supply
    • balance_of -> ft_balance_of
  • rename func on FungibleTokenCoreResolveTransfer
    • resolve_transfer -> ft_resolve_transfer
  • rename func on FungibleTokenReceiver
    • on_ft_transfer -> ft_on_transfer
pub trait FungibleTokenCore {
    #[payable]
    fn ft_transfer(&mut self, receiver_id: ValidAccountId, amount: U128, memo: Option<String>);

    #[payable]
    fn ft_transfer_call(
        &mut self,
        receiver_id: ValidAccountId,
        amount: U128,
        msg: String,
        memo: Option<String>,
    ) -> Promise;

    fn ft_total_supply(&self) -> U128;

    fn ft_balance_of(&self, account_id: ValidAccountId) -> U128;
}

pub trait FungibleTokenReceiver {
    fn ft_on_transfer(
        &mut self,
        sender_id: ValidAccountId,
        amount: U128,
        msg: String,
    ) -> PromiseOrValue<TokenAmount>;
}

pub trait FungibleTokenCoreResolveTransfer {
    #[private]
    fn ft_resolve_transfer(
        &mut self,
        sender_id: ValidAccountId,
        receiver_id: ValidAccountId,
        amount: U128,
        #[callback_result] used_amount: CallbackResult<U128>
    ) -> U128;
}

(code comments were stripped out to focus on the code)

@oysterpack
Copy link

oysterpack commented Jan 8, 2021

REJECTED: Change Proposal: Replace primitive types with domain specific wrappers

REJECTED REASON: the proposal belongs in Rust reference implementation

On the wire, the underlying types will be marshalled in low level primitive format to be as efficient as possible. However, the contract interface should be strongly domain typed to clearly express purpose.

I propose the following domain types:

  • TokenAmount(U128)
  • Memo(String)
  • TransferCallData(Vec<u8>)
    • the underlying has been changed from text (String) to binary (Vec) from the original proposal for efficiency reasons - per @miohtama suggestion
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct TokenAmount(pub U128);

impl From<u128> for TokenAmount {
    fn from(value: u128) -> Self {
        Self(value.into())
    }
}

impl TokenAmount {
    pub fn value(&self) -> u128 {
        self.0 .0
    }
}

impl Deref for TokenAmount {
    type Target = u128;

    fn deref(&self) -> &Self::Target {
        &self.0 .0
    }
}

impl DerefMut for TokenAmount {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0 .0
    }
}

impl Display for TokenAmount {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0 .0.fmt(f)
    }
}

impl PartialOrd for TokenAmount {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        self.value().partial_cmp(&other.value())
    }
}

impl Ord for TokenAmount {
    fn cmp(&self, other: &Self) -> Ordering {
        self.value().cmp(&other.value())
    }
}

impl Eq for TokenAmount {}

/// > Similarly to bank transfer and payment orders, the memo argument allows to reference transfer
/// > to other event (on-chain or off-chain). It is a schema less, so user can use it to reference
/// > an external document, invoice, order ID, ticket ID, or other on-chain transaction. With memo
/// > you can set a transfer reason, often required for compliance.
/// >
/// > This is also useful and very convenient for implementing FATA (Financial Action Task Force)
/// > guidelines (section 7(b) ). Especially a requirement for VASPs (Virtual Asset Service Providers)
/// > to collect and transfer customer information during transactions. VASP is any entity which
/// > provides to a user token custody, management, exchange or investment services.
/// > With ERC-20 (and NEP-21) it is not possible to do it in atomic way. With memo field, we can
/// > provide such reference in the same transaction and atomically bind it to the money transfer.
///
/// - https://github.com/near/NEPs/issues/136
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct Memo(pub String);

impl Deref for Memo {
    type Target = str;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl Display for Memo {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0.fmt(f)
    }
}

/// > The mint, send and burn processes can all make use of a data and operatorData fields which are
/// > passed to any movement (mint, send or burn). Those fields may be empty for simple use cases,
/// > or they may contain valuable information related to the movement of tokens, similar to
/// > information attached to a bank transfer by the sender or the bank itself.
/// > The use of a data field is equally present in other standard proposals such as EIP-223, and
/// > was requested by multiple members of the community who reviewed this standard.
/// >
/// - https://eips.ethereum.org/EIPS/eip-777#data
#[derive(BorshDeserialize, BorshSerialize, Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct TransferCallData(pub Vec<u8>);

impl Deref for TransferCallData {
    type Target = [u8];

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

@evgenykuzyakov
Copy link
Contributor Author

Regarding renaming to ft_ in #141 (comment)
Let's also rename on_ft_receive to ft_on_receive.

@oysterpack
Copy link

Regarding renaming to ft_ in #141 (comment)
Let's also rename on_ft_receive to ft_on_receive.

done

@oysterpack
Copy link

oysterpack commented Jan 8, 2021

REJECTED: Change Proposal: drop msg and add optional attributes to transfer_call

REJECTED REASON: msg use case was misunderstood - see @evgenykuzyakov explanation below

We can drop msg because memo should be sufficient. Furthermore, memo's purpose should be explicity defined.

  • memo is expected to be human friendly memo that is meant for display purposes

For extensibility, optional attributes can be specified to pass along arbitrary data to FungibleTokenReceiver as part
of transfer_call workflow. For example, the attributes may be used to provide instructions on how to process the
transfer on the receiver side.

  • If any data is binary, then the attribute value would be base64 encoded.
  • NOTE: attributes were intentionally not added for the simple transfer use case because I don't see it providing value.
    Attributes are passed through on the transfer_call to provide more context for the FungibleTokenReceiver to process
    the transfer.
pub trait FungibleTokenCore {
    /// - `msg` arg has been dropped
    /// - `memo` - is expected to be human friendly memo that is meant for display purposes
    #[payable]
    fn ft_transfer(
        &mut self,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        memo: Option<String>,           
    );

    /// - `msg` arg has been dropped
    /// - `memo` - is expected to be human friendly memo that is meant for display purposes
    /// - added new `attributes` arg
    #[payable]
    fn ft_transfer_call(
        &mut self,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        memo: Option<String>,
        attributes: Option<HashMap<String, String>>,
    ) -> Promise;

    fn ft_total_supply(&self) -> TokenAmount;

    fn ft_balance_of(&self, account_id: ValidAccountId) -> TokenAmount;
}

/// Receiver of the Fungible Token for `ft_transfer_and_notify` calls.
pub trait FungibleTokenReceiver {

    /// - `msg` arg has been dropped
    /// - `memo` - is expected to be human friendly memo that is meant for display purposes
    /// - added new `attributes` arg
    fn ft_on_transfer(
        &mut self,
        sender_id: ValidAccountId,
        amount: TokenAmount,
        memo: Option<String>,
        attributes: Option<HashMap<String, String>>,
    ) -> PromiseOrValue<TokenAmount>;
}

@oysterpack
Copy link

oysterpack commented Jan 8, 2021

REJECTED: Change Proposal: simplify transfer_call workflow by removing the "refund" step - drop

REJECTED REASON: transfer use case was not well understood

FungibleTokenCoreResolveTransfer
The purpose of the transfer_call workflow is to notify the receiver of the transfer, which enables the receiver to
react and take some action. If a refund is part of the receiver business logic, then the receiver can simply apply the
refund using either simple transfer or even via another transfer_call.

Let's keep the transfer_call workflow as lightweight and simple as possible. I would expect that a significant
percentage of the transfer_callwill not require a refund.

Interface changes

  • FungibleTokenReceiver::ft_on_transfer() return type is changed to ()
/// Receiver of the Fungible Token for `ft_transfer_and_notify` calls.
pub trait FungibleTokenReceiver {

    /// return type changed to ()
    fn ft_on_transfer(
        &mut self,
        sender_id: ValidAccountId,
        amount: TokenAmount,
        memo: Option<String>,
        attributes: Option<HashMap<String, String>>,
    ) ;
}

@evgenykuzyakov
Copy link
Contributor Author

evgenykuzyakov commented Jan 8, 2021

msg is generic enough to represent a payload. It can be a JSON on itself (similar to NEP-122 implementations) or a base64 encoded Borsh message (hard to verify in the wallet). attributes is a subset of the msg. I'm pretty sure msg is required to represent an action and another guard for human errors.
memo purpose is completely different from the msg, so we shouldn't confuse 2 of them.

@evgenykuzyakov
Copy link
Contributor Author

Dropping refund will lead to human mistakes where the unsupported tokens are locked on the receiver's contract. Also it will lead to lost tokens due to insufficient gas. Since we doing refund anyway for failed promises. The result makes it explicit without extra gas cost.

@oysterpack
Copy link

msg is generic enough to represent a payload. It can be a JSON on itself (similar to NEP-122 implementations) or a base64 encoded Borsh message (hard to verify in the wallet). attributes is a subset of the msg. I'm pretty sure msg is required to represent an action and another guard for human errors.
memo purpose is completely different from the msg, so we shouldn't confuse 2 of them.

I see ... I misinterpreted the semantics for msg ...

To clarify, you mean:

  • msg - is "data" intended for the FungibleTokenReceiver
  • 'memo` is intended for human display

In that case I agree with you, but I will propose the following minor changes:

  1. rename msg to data to make the intent more clear.
  2. make it optional: Option<String>

@oysterpack
Copy link

Dropping refund will lead to human mistakes where the unsupported tokens are locked on the receiver's contract.

I am going based on the assumption that the receiver account must be pre-registered with the FT contract. Thus, that should prevent this class of human mistakes because the receiver contract must support the tokens since they must actively pre-register. Or is there another type of human mistake you are thinking of.

Also it will lead to lost tokens due to insufficient gas. Since we doing refund anyway for failed promises.

I guess that depends on the purpose of the transfer_call. To me it says, transfer the tokens and then notify the receiver of the transfer. In addition, because the tokens are not locked, there is potential for race conditions. These are edge cases, but when it comes to "financial" transactions, race conditions are not acceptable. Financial transactions need to be "transactional". If the receiver call fails, that is separate concern then the transfer. I think we are trying to cover too many transfer use cases with transfer_call, which we should keep separate.

If we want to support refunds and confirmations, then the transferred funds should be locked - either via vault based approach (#122) or lock based mechanism used in (#110) for transfer confirmations.

What are your thoughts on adding transfer functions for vault based transfer and transfer confirmation:

    /// tokens are locked until the receiver contract confirms the transfer
    fn ft_confirm_transfer(
        &mut self,
        recipient: ValidAccountId,
        amount: TokenAmount,
        memo: Option<String>,
        data: Option<String>,
    ) -> Promise;

    fn ft_transfer_with_vault(
        &mut self,
        recipient: ValidAccountId,
        amount: TokenAmount,
        memo: Option<String>,
        data: Option<String>,
    ) -> Promise;

Having separate transfer workflows provides more flexibility and applications can choose the transfer mechanism that best fits their use case.

The result makes it explicit without extra gas cost.

Can you please elaborate on this. How much extra?

@evgenykuzyakov
Copy link
Contributor Author

I am going based on the assumption that the receiver account must be pre-registered with the FT contract. Thus, that should prevent this class of human mistakes because the receiver contract must support the tokens since they must actively pre-register. Or is there another type of human mistake you are thinking of.

Pre-registration does indeed guards against unwanted usage, but it doesn't block someone from registering an unsupported contract.

The result makes it explicit without extra gas cost.

If the receiver contract is required to return the used amount, then it serve 2 cases:

  • the contract can't accidentally accept funds (maybe a different standard) that didn't panic.
  • The token contract will manage the refund for failed promises use-case anyway. So if we use return value from the promise, we don't increase the amount of gas by a lot and we don't increase the amount of cross-contract calls at all. But it covers the Uniswap use-case explained in Interactive Fungible Token #136 (comment)

rename msg to data to make the intent more clear.
make it optional: Option<String>

I don't think data should be optional. The transfer call should always receive this argument. It's another guard on the correct receiving side. E.g. uniswap may use swap, while bananafarm is convert_to_cucumbers

@evgenykuzyakov
Copy link
Contributor Author

I guess that depends on the purpose of the transfer_call. To me it says, transfer the tokens and then notify the receiver of the transfer. In addition, because the tokens are not locked, there is potential for race conditions. These are edge cases, but when it comes to "financial" transactions, race conditions are not acceptable. Financial transactions need to be "transactional". If the receiver call fails, that is separate concern then the transfer. I think we are trying to cover too many transfer use cases with transfer_call, which we should keep separate.

Being able to return the amount of tokens received doesn't complicate the receiver's contract implementation. It still transactional, since with NEAR async environment, the receiver's contract relies on the inner balance value, instead of using balance_of feature of the token contract. So when the new tokens comes in and the callback arrives, the receiver's token just updates its inner balance.

@oysterpack
Copy link

@evgenykuzyakov - thanks that clears things up for me and convinces me to change my vote - I am going to close out that change proposal.

But what are your thoughts on ft_confirm_transfer and ft_transfer_with_vault ?

@miohtama
Copy link

miohtama commented Jan 10, 2021

Shoudn't data be binary? Generally, any inefficiencies in contract communications should be avoided. Most payloads between the contract calls are serialised binary data, likely function arguments and hashes, if you look how the existing Ethereum DeFi ecosystem is operating.

@oysterpack
Copy link

Shoudn't data be binary? Generally, any inefficiencies in contract communications should be avoided. Most payloads between the contract calls are serialised binary data, likely function arguments and hashes, if you look how the existing Ethereum DeFi ecosystem is operating.

makes sense - data should be serialized as efficiently as possible in order to scale efficiently ... recommending Borsh serialization as the preferred binary transport format

@oysterpack
Copy link

Change Proposal: Replace primitive types with domain specific wrappers has been updated with 2 more domain types:

  • TokenAmount(U128)
  • Memo(String)
  • TransferCallData(Vec<u8>)

@oysterpack
Copy link

@miohtama has made me rethink whether or not transfer call data should be optional ... data may not be required for simple use cases. To make the simple use case as efficient as possible, then call data should be made optional. If the receiver requires call data to process the transfer, then it should fail the transfer if the call data is not specified or not valid.

@evgenykuzyakov cited the following benefit for making call data required

I don't think data should be optional. The transfer call should always receive this argument. It's another guard on the correct receiving side. E.g. uniswap may use swap, while bananafarm is convert_to_cucumbers

How does making the call data required add value as an extra guard on the receiver side? The receiver side is already guarded by the following:

  • the receiver contract account mus be pre-registered with the FT contract
  • the receiver contract must implement the FungibleTokenReceiver::ft_on_transfer interface
  • receiver side business logic - FungibleTokenReceiver::ft_on_transfer should fail if call data is not valid

What percentage of transfer use cases will require call data? I would be convinced to make it required if more than 90% of the transfer call use cases will require call data. Otherwise, let's make it optional.

@oysterpack
Copy link

oysterpack commented Jan 11, 2021

REJECTED: Change Proposal: Add contract function to burn tokens

REJECT REASON: this belongs in separate standard as an optional extension

pub trait FungibleTokenCore {
    /// Destroys specified amount of tokens from the predecessor account, reducing the total supply.
    ///
    /// # Panics
    /// - if there is no attached deposit
    /// - if account is not registered
    /// - if amount is zero
    /// - if the account has insufficient funds to fulfill the request
    ///
    /// #\[payable\]
    fn ft_burn(&mut self, amount: TokenAmount, memo: Option<Memo>);
    
    // ...
}

@oysterpack
Copy link

oysterpack commented Jan 11, 2021

What should be the goals and principles of the core FT standard? Here are my thoughts:

  1. it should be as simple as possible - it should be simple to understand and explain
  2. at the same time it should be as robust as possible to cover 99% of the use cases
  3. it should be as secure as possible
  4. it should be as efficient as possible and scalable

Are there any goals or criteria that I am missing?

Using the above guidelines and criteria, helps me to focus my thoughts to better decide what should be in scope for the core FT standard. Without clear goals and criteria, the discussion will tend to drift and bloat. I am guilty of it ... For example, when applying these guidelines it tells me that ft_confirm_transfer and ft_transfer_with_vault do not belong in the core standard for the following reasons:

  • too complex for the core standard
    • both require more state management on the token contract
    • vault transfer requires additional cross contract call for vault withdrawal
  • less efficient and more costly to implement in terms of resources

That being said, both ft_confirm_transfer and ft_transfer_with_vault do provide value but fall outside the scope of the core FT standard.

Examples of applying the above principles and guidelines:

  • applying efficiency principle - transfer call data should be binary and optional. We need to consider scale, scaling to millions of TPS (FT transfers per second). Every efficiency gain is huge when considering scale.
  • applying robustness principle - as part of the core standard we need the ability to burn tokens
  • applying security principle - requiring payment (of at least 1 yoctoNEAR) is the cost of doing business as securely as possible
    • I was thinking about adding methods to track NEAR deposits used for transfers in order to to support refunding the NEAR deposits - but after applying principles of simplicity and efficiency - I decided against it

To bring this full circle, I believe we are pretty close (if not there already) at arriving at the core FT standard:

use near_sdk::{
    borsh::{self, BorshDeserialize, BorshSerialize},
    json_types::{ValidAccountId, U128},
    serde::{Deserialize, Serialize},
    Promise, PromiseOrValue,
};
use std::{
    cmp::Ordering,
    fmt::{self, Display, Formatter},
    ops::{Deref, DerefMut},
};

/// Defines the standard interface for the core Fungible Token contract
/// - [NEP-141](https://github.com/near/NEPs/issues/141)
///
/// The core standard supports the following features:
/// - [simple token transfers](FungibleTokenCore::ft_transfer)
/// - [token transfers between contracts](FungibleTokenCore::ft_transfer_call)
/// - [burning tokens](FungibleTokenCore::ft_burn)
/// - accounting for [total token supply](FungibleTokenCore::ft_total_supply) and
///   [account balances](FungibleTokenCore::ft_balance_of)
///
/// ## Notes
/// - it doesn't include token metadata standard that will be covered by a separate NEP, because the
///   metadata may evolve.
/// - it also doesn't include account registration standard that also should be covered by a separate
///   NEP because it can be reused for other contract.
///
/// ### Security
/// Requirement for accept attached deposits (#\[payable\])
/// Due to the nature of function-call permission access keys on NEAR protocol, the method that
/// requires an attached deposit can't be called by the restricted access key. If the token contract
/// requires an attached deposit of at least 1 yoctoNEAR on transfer methods, then the function-call
/// restricted access key will not be able to call them without going through the wallet confirmation.
/// This prevents some attacks like fishing through an authorization to a token contract.
///
/// This 1 yoctoNEAR is not enforced by this standard, but is encouraged to do. While ability to
/// receive attached deposit is enforced by this token.
///
/// ### Transfer Call Refunds
/// If the receiver contract is malicious or incorrectly implemented, then the receiver's promise
/// result may be invalid and the required balance may not be available on the receiver's account.
/// In this case the refund can't be provided provided to the sender. This is prevented by #122
/// standard that locks funds into a temporary vault and prevents receiver from overspending the
/// funds and later retuning invalid value. But if this flaw exist in this standard, it's not an
/// issue for the sender account. It only affects the transfer amount and the receiver's account
/// balance. The receiver can't overspend tokens from the sender outside of sent amount, so this
/// standard should be considered as safe as #122
///
pub trait FungibleTokenCore {
    /// Enables simple transfer between accounts.
    ///
    /// - Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id`.
    /// - Both accounts should be registered with the contract for transfer to succeed.
    /// - Method is required to be able to accept attached deposits - to not panic on attached deposit.
    ///   See security section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    ///
    /// ## Panics
    /// - if there is no attached deposit
    /// - if either sender or receiver accounts are not registered
    /// - if amount is zero
    /// - if the sender account has insufficient funds to fulfill the request
    ///
    /// #\[payable\]
    fn ft_transfer(&mut self, receiver_id: ValidAccountId, amount: TokenAmount, memo: Option<Memo>);

    /// Transfer to a contract with a callback.
    ///
    /// Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id`
    /// account. Then calls [`FungibleTokenReceiver::ft_on_transfer`] method on `receiver_id` contract
    /// and attaches a callback to resolve this transfer.
    /// [`FungibleTokenReceiver::ft_on_transfer`] method should return the amount of tokens used by
    /// the receiver contract, the remaining tokens should be refunded to the `predecessor_account_id`
    /// at the resolve transfer callback.
    ///
    /// Token contract should pass all the remaining unused gas to [`FungibleTokenReceiver::ft_on_transfer`]
    ///
    /// Malicious or invalid behavior by the receiver's contract:
    /// - If the receiver contract promise fails or returns invalid value, the full transfer amount
    ///   should be refunded.
    /// - If the receiver contract overspent the tokens, and the `receiver_id` balance is lower
    ///   than the required refund amount, the remaining balance should be refunded.
    ///
    /// Both accounts should be registered with the contract for transfer to succeed.
    /// Method is required to be able to accept attached deposits - to not panic on attached deposit. See Security
    /// section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver contract. This contract will be called.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `data` - a string message that will be passed to `ft_on_transfer` contract call.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    /// Returns a promise to resolve transfer call which will return the used amount (see suggested trait to resolve
    /// transfer).
    ///
    /// ## Panics
    /// - if there is no attached deposit
    /// - if either sender or receiver accounts are not registered
    /// - if amount is zero
    /// - if the sender account has insufficient funds to fulfill the transfer request
    ///
    /// #\[payable\]
    fn ft_transfer_call(
        &mut self,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        data: Option<TransferCallData>,
        memo: Option<Memo>,
    ) -> Promise;

    /// Destroys specified amount of tokens from the predecessor account, reducing the total supply.
    ///
    /// # Panics
    /// - if there is no attached deposit
    /// - if account is not registered
    /// - if amount is zero
    /// - if the account has insufficient funds to fulfill the transfer request
    ///
    /// #\[payable\]
    fn ft_burn(&mut self, amount: TokenAmount, memo: Option<Memo>);

    fn ft_total_supply(&self) -> TokenAmount;

    fn ft_balance_of(&self, account_id: ValidAccountId) -> TokenAmount;
}

/// Receiver of the Fungible Token for [`FungibleTokenCore::ft_transfer_call`] calls.
pub trait FungibleTokenReceiver {
    /// Callback to receive tokens.
    ///
    /// Called by fungible token contract `env::predecessor_account_id` after `transfer_call` was initiated by
    /// `sender_id` of the given `amount` with the transfer message given in `msg` field.
    /// The `amount` of tokens were already transferred to this contract account and ready to be used.
    ///
    /// The method should return the amount of tokens that are used/accepted by this contract from the transferred
    /// amount. Examples:
    /// - The transferred amount was `500`, the contract completely takes it and should return `500`.
    /// - The transferred amount was `500`, but this transfer call only needs `450` for the action passed in the `msg`
    ///   field, then the method should return `450`.
    /// - The transferred amount was `500`, but the action in `msg` field has expired and the transfer should be
    ///   cancelled. The method should return `0` or panic.
    ///
    /// Arguments:
    /// - `sender_id` - the account ID that initiated the transfer.
    /// - `amount` - the amount of tokens that were transferred to this account.
    /// - `msg` - a string message that was passed with this transfer call.
    ///
    /// Returns the amount of tokens that are used/accepted by this contract from the transferred amount.
    fn ft_on_transfer(
        &mut self,
        sender_id: ValidAccountId,
        amount: TokenAmount,
        msg: Option<TransferCallData>,
    ) -> PromiseOrValue<TokenAmount>;
}

/// Suggested Trait to handle the callback on fungible token contract to resolve transfer.
/// It's not a public interface, so fungible token contract can implement it differently.
pub trait FungibleTokenCoreResolveTransferCall {
    /// Callback to resolve transfer.
    /// Private method (`env::predecessor_account_id == env::current_account_id`).
    ///
    /// Called after the receiver handles the transfer call and returns value of used amount in `U128`.
    ///
    /// This method should get `used_amount` from the receiver's promise result and refund the remaining
    /// `amount - used_amount` from the receiver's account back to the `sender_id` account.
    /// Methods returns the amount tokens that were spent from `sender_id` after the refund
    /// (`amount - min(receiver_balance, used_amount)`)
    ///
    /// Arguments:
    /// - `sender_id` - the account ID that initiated the transfer.
    /// - `receiver_id` - the account ID of the receiver contract.
    /// - `amount` - the amount of tokens that were transferred to receiver's account.
    ///
    /// Promise results:
    /// - `used_amount` - the amount of tokens that were used by receiver's contract. Received from `on_ft_receive`.
    ///   `used_amount` should be `U128` in range from `0` to `amount`. All other invalid values are considered to be
    ///   equal to `0`.
    ///
    /// Returns the amount of tokens that were spent from the `sender_id` account. Note, this value might be different
    /// from the `used_amount` returned by the receiver contract, in case the refunded balance is not available on the
    /// receiver's account.
    ///
    /// #\[private\]
    fn resolve_transfer_call(
        &mut self,
        sender_id: ValidAccountId,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        // NOTE: #[callback_result] is not supported yet and has to be handled using lower level interface.
        //
        // #[callback_result]
        // used_amount: CallbackResult<TokenAmount>,
    );
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct TokenAmount(pub U128);

impl From<u128> for TokenAmount {
    fn from(value: u128) -> Self {
        Self(value.into())
    }
}

impl TokenAmount {
    pub fn value(&self) -> u128 {
        self.0 .0
    }
}

impl Deref for TokenAmount {
    type Target = u128;

    fn deref(&self) -> &Self::Target {
        &self.0 .0
    }
}

impl DerefMut for TokenAmount {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0 .0
    }
}

impl Display for TokenAmount {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0 .0.fmt(f)
    }
}

impl PartialOrd for TokenAmount {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        self.value().partial_cmp(&other.value())
    }
}

impl Ord for TokenAmount {
    fn cmp(&self, other: &Self) -> Ordering {
        self.value().cmp(&other.value())
    }
}

impl Eq for TokenAmount {}

/// > Similarly to bank transfer and payment orders, the memo argument allows to reference transfer
/// > to other event (on-chain or off-chain). It is a schema less, so user can use it to reference
/// > an external document, invoice, order ID, ticket ID, or other on-chain transaction. With memo
/// > you can set a transfer reason, often required for compliance.
/// >
/// > This is also useful and very convenient for implementing FATA (Financial Action Task Force)
/// > guidelines (section 7(b) ). Especially a requirement for VASPs (Virtual Asset Service Providers)
/// > to collect and transfer customer information during transactions. VASP is any entity which
/// > provides to a user token custody, management, exchange or investment services.
/// > With ERC-20 (and NEP-21) it is not possible to do it in atomic way. With memo field, we can
/// > provide such reference in the same transaction and atomically bind it to the money transfer.
///
/// - https://github.com/near/NEPs/issues/136
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct Memo(pub String);

impl Deref for Memo {
    type Target = str;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl Display for Memo {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0.fmt(f)
    }
}

/// > The mint, send and burn processes can all make use of a data and operatorData fields which are
/// > passed to any movement (mint, send or burn). Those fields may be empty for simple use cases,
/// > or they may contain valuable information related to the movement of tokens, similar to
/// > information attached to a bank transfer by the sender or the bank itself.
/// > The use of a data field is equally present in other standard proposals such as EIP-223, and
/// > was requested by multiple members of the community who reviewed this standard.
/// >
/// - https://eips.ethereum.org/EIPS/eip-777#data
#[derive(BorshDeserialize, BorshSerialize, Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct TransferCallData(pub Vec<u8>);

impl Deref for TransferCallData {
    type Target = [u8];

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

NOTES

Events should be defined as part of the standard. However, because NEAR does not currently support events, events have been intentionally left out of scope. Once NEAR officially supports events, then events should be reconsidered.

@evgenykuzyakov
Copy link
Contributor Author

makes sense - data should be serialized as efficiently as possible in order to scale efficiently ... recommending Borsh serialization as the preferred binary transport format

Considering JSON serializes Vec<u8> as an array of integers (e.g. [65, 92, 64, 53]) it's clearly a mistake to use. For binary data we use base64 encoding. But Base64 is just a subset of a string. I think the data confusion comes from not understanding the usecase. Also trying to optimize the arguments size and serialization/deserialization right now is a mistake. The overhead of JSON serialization/deserialization is minor for small data size (e.g. less than 100 bytes). See near/nearcore#1829 for comparison.

Explaining data or msg argument

When you call transfer_call towards a contract you are effectively attaching a deposit of a fungible token and making a function call on this contract. It's similar to making a function call on a contract and attaching native NEAR, except you attach this particular fungible token.

If we follow this logic, then for a function call you specify the following:

  • predecessor_id
  • receiver_id
  • method_name
  • arguments
  • deposit
  • prepaid_gas

Now for a fungible token function call using transfer_call, you have the following:

  • predecessor_id - since actual predecessor is token contract, the sender account ID is given using sender_id.
  • receiver_id - the receiver contract
  • method_name - there is no way to specify a method name using transfer_call API, so it's predefined to ft_on_transfer. But the receiving contract needs to know why transfer was received and which method has to be executed. It's possible that there is only one action that needs to handle receiving tokens, so method_name can be implied. But also possible that there are more than one action available, e.g. swap or account_deposit for uniswap contract. If there are more than one action, then we need to use msg field to specify which action to take.
  • arguments - if the receiving contract needs any information or data beyond transfer amount, then msg is useful to include them.
  • deposit - this comes from transfer amount
  • prepaid_gas - all the remaining gas is forwarded to the receiver's call.

To summarize the 2 main use cases for msg are the method_name and args that are need to process the transfer on the receiving side.
I'd argue that using one of them is usually required, but obviously there are some cases that don't require arguments and there is only one action on the receiver. Still making them optional doesn't save much, but can be confused with regular transfer call.

> ft_burn

Burn/mint functionality is not required for all token contracts, so it should go into a separate standard.

@oysterpack
Copy link

@evgenykuzyakov - thanks for fully explaining transfer_call use case - I didn't see the full picture until now. Good catch on the JSON binary serialization being inefficient. I agree with your comments and have applied your suggestions to the latest and greatest revision:

use near_sdk::{
    borsh::{self, BorshDeserialize, BorshSerialize},
    json_types::{ValidAccountId, U128},
    serde::{Deserialize, Serialize},
    Promise, PromiseOrValue,
};
use std::{
    cmp::Ordering,
    fmt::{self, Display, Formatter},
    ops::{Deref, DerefMut},
};

/// Defines the standard interface for the core Fungible Token contract
/// - [NEP-141](https://github.com/near/NEPs/issues/141)
///
/// The core standard supports the following features:
/// - [simple token transfers](FungibleTokenCore::ft_transfer)
/// - [token transfers between contracts](FungibleTokenCore::ft_transfer_call)
/// - accounting for [total token supply](FungibleTokenCore::ft_total_supply) and
///   [account balances](FungibleTokenCore::ft_balance_of)
///
/// ## Notes
/// - it doesn't include token metadata standard that will be covered by a separate NEP, because the
///   metadata may evolve.
/// - it also doesn't include account registration standard that also should be covered by a separate
///   NEP because it can be reused for other contract.
///
/// ### Security
/// Requirement for accept attached deposits (#\[payable\])
/// Due to the nature of function-call permission access keys on NEAR protocol, the method that
/// requires an attached deposit can't be called by the restricted access key. If the token contract
/// requires an attached deposit of at least 1 yoctoNEAR on transfer methods, then the function-call
/// restricted access key will not be able to call them without going through the wallet confirmation.
/// This prevents some attacks like fishing through an authorization to a token contract.
///
/// This 1 yoctoNEAR is not enforced by this standard, but is encouraged to do. While ability to
/// receive attached deposit is enforced by this token.
///
/// ### Transfer Call Refunds
/// If the receiver contract is malicious or incorrectly implemented, then the receiver's promise
/// result may be invalid and the required balance may not be available on the receiver's account.
/// In this case the refund can't be provided provided to the sender. This is prevented by #122
/// standard that locks funds into a temporary vault and prevents receiver from overspending the
/// funds and later retuning invalid value. But if this flaw exist in this standard, it's not an
/// issue for the sender account. It only affects the transfer amount and the receiver's account
/// balance. The receiver can't overspend tokens from the sender outside of sent amount, so this
/// standard should be considered as safe as #122
///
pub trait FungibleTokenCore {
    /// Enables simple transfer between accounts.
    ///
    /// - Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id`.
    /// - Both accounts should be registered with the contract for transfer to succeed.
    /// - Method is required to be able to accept attached deposits - to not panic on attached deposit.
    ///   See security section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    ///
    /// ## Panics
    /// - if there is no attached deposit
    /// - if either sender or receiver accounts are not registered
    /// - if amount is zero
    /// - if the sender account has insufficient funds to fulfill the request
    ///
    /// #\[payable\]
    fn ft_transfer(&mut self, receiver_id: ValidAccountId, amount: TokenAmount, memo: Option<Memo>);

    /// Transfer to a contract with a callback.
    ///
    /// Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id`
    /// account. Then calls [`FungibleTokenReceiver::ft_on_transfer`] method on `receiver_id` contract
    /// and attaches a callback to resolve this transfer.
    /// [`FungibleTokenReceiver::ft_on_transfer`] method should return the amount of tokens used by
    /// the receiver contract, the remaining tokens should be refunded to the `predecessor_account_id`
    /// at the resolve transfer callback.
    ///
    /// Token contract should pass all the remaining unused gas to [`FungibleTokenReceiver::ft_on_transfer`]
    ///
    /// Malicious or invalid behavior by the receiver's contract:
    /// - If the receiver contract promise fails or returns invalid value, the full transfer amount
    ///   should be refunded.
    /// - If the receiver contract overspent the tokens, and the `receiver_id` balance is lower
    ///   than the required refund amount, the remaining balance should be refunded.
    ///
    /// Both accounts should be registered with the contract for transfer to succeed.
    /// Method is required to be able to accept attached deposits - to not panic on attached deposit. See Security
    /// section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver contract. This contract will be called.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `data` - a string message that will be passed to `ft_on_transfer` contract call.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    /// Returns a promise to resolve transfer call which will return the used amount (see suggested trait to resolve
    /// transfer).
    ///
    /// ## Panics
    /// - if there is no attached deposit
    /// - if either sender or receiver accounts are not registered
    /// - if amount is zero
    /// - if the sender account has insufficient funds to fulfill the transfer request
    ///
    /// #\[payable\]
    fn ft_transfer_call(
        &mut self,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        data: TransferCallData,
        memo: Option<Memo>,
    ) -> Promise;

    fn ft_total_supply(&self) -> TokenAmount;

    fn ft_balance_of(&self, account_id: ValidAccountId) -> TokenAmount;
}

/// Receiver of the Fungible Token for [`FungibleTokenCore::ft_transfer_call`] calls.
pub trait FungibleTokenReceiver {
    /// Callback to receive tokens.
    ///
    /// Called by fungible token contract `env::predecessor_account_id` after `transfer_call` was initiated by
    /// `sender_id` of the given `amount` with the transfer message given in `msg` field.
    /// The `amount` of tokens were already transferred to this contract account and ready to be used.
    ///
    /// The method should return the amount of tokens that are used/accepted by this contract from the transferred
    /// amount. Examples:
    /// - The transferred amount was `500`, the contract completely takes it and should return `500`.
    /// - The transferred amount was `500`, but this transfer call only needs `450` for the action passed in the `msg`
    ///   field, then the method should return `450`.
    /// - The transferred amount was `500`, but the action in `msg` field has expired and the transfer should be
    ///   cancelled. The method should return `0` or panic.
    ///
    /// Arguments:
    /// - `sender_id` - the account ID that initiated the transfer.
    /// - `amount` - the amount of tokens that were transferred to this account.
    /// - `msg` - a string message that was passed with this transfer call.
    ///
    /// Returns the amount of tokens that are used/accepted by this contract from the transferred amount.
    fn ft_on_transfer(
        &mut self,
        sender_id: ValidAccountId,
        amount: TokenAmount,
        data: TransferCallData,
    ) -> PromiseOrValue<TokenAmount>;
}

/// Suggested Trait to handle the callback on fungible token contract to resolve transfer.
/// It's not a public interface, so fungible token contract can implement it differently.
pub trait FungibleTokenCoreResolveTransferCall {
    /// Callback to resolve transfer.
    /// Private method (`env::predecessor_account_id == env::current_account_id`).
    ///
    /// Called after the receiver handles the transfer call and returns value of used amount in `U128`.
    ///
    /// This method should get `used_amount` from the receiver's promise result and refund the remaining
    /// `amount - used_amount` from the receiver's account back to the `sender_id` account.
    /// Methods returns the amount tokens that were spent from `sender_id` after the refund
    /// (`amount - min(receiver_balance, used_amount)`)
    ///
    /// Arguments:
    /// - `sender_id` - the account ID that initiated the transfer.
    /// - `receiver_id` - the account ID of the receiver contract.
    /// - `amount` - the amount of tokens that were transferred to receiver's account.
    ///
    /// Promise results:
    /// - `used_amount` - the amount of tokens that were used by receiver's contract. Received from `on_ft_receive`.
    ///   `used_amount` should be `U128` in range from `0` to `amount`. All other invalid values are considered to be
    ///   equal to `0`.
    ///
    /// Returns the amount of tokens that were spent from the `sender_id` account. Note, this value might be different
    /// from the `used_amount` returned by the receiver contract, in case the refunded balance is not available on the
    /// receiver's account.
    ///
    /// #\[private\]
    fn resolve_transfer_call(
        &mut self,
        sender_id: ValidAccountId,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        // NOTE: #[callback_result] is not supported yet and has to be handled using lower level interface.
        //
        // #[callback_result]
        // used_amount: CallbackResult<TokenAmount>,
    );
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct TokenAmount(pub U128);

impl From<u128> for TokenAmount {
    fn from(value: u128) -> Self {
        Self(value.into())
    }
}

impl TokenAmount {
    pub fn value(&self) -> u128 {
        self.0 .0
    }
}

impl Deref for TokenAmount {
    type Target = u128;

    fn deref(&self) -> &Self::Target {
        &self.0 .0
    }
}

impl DerefMut for TokenAmount {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0 .0
    }
}

impl Display for TokenAmount {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0 .0.fmt(f)
    }
}

impl PartialOrd for TokenAmount {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        self.value().partial_cmp(&other.value())
    }
}

impl Ord for TokenAmount {
    fn cmp(&self, other: &Self) -> Ordering {
        self.value().cmp(&other.value())
    }
}

impl Eq for TokenAmount {}

/// > Similarly to bank transfer and payment orders, the memo argument allows to reference transfer
/// > to other event (on-chain or off-chain). It is a schema less, so user can use it to reference
/// > an external document, invoice, order ID, ticket ID, or other on-chain transaction. With memo
/// > you can set a transfer reason, often required for compliance.
/// >
/// > This is also useful and very convenient for implementing FATA (Financial Action Task Force)
/// > guidelines (section 7(b) ). Especially a requirement for VASPs (Virtual Asset Service Providers)
/// > to collect and transfer customer information during transactions. VASP is any entity which
/// > provides to a user token custody, management, exchange or investment services.
/// > With ERC-20 (and NEP-21) it is not possible to do it in atomic way. With memo field, we can
/// > provide such reference in the same transaction and atomically bind it to the money transfer.
///
/// - https://github.com/near/NEPs/issues/136
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct Memo(pub String);

impl Deref for Memo {
    type Target = str;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl Display for Memo {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0.fmt(f)
    }
}

/// > The mint, send and burn processes can all make use of a data and operatorData fields which are
/// > passed to any movement (mint, send or burn). Those fields may be empty for simple use cases,
/// > or they may contain valuable information related to the movement of tokens, similar to
/// > information attached to a bank transfer by the sender or the bank itself.
/// > The use of a data field is equally present in other standard proposals such as EIP-223, and
/// > was requested by multiple members of the community who reviewed this standard.
/// >
/// - https://eips.ethereum.org/EIPS/eip-777#data
#[derive(BorshDeserialize, BorshSerialize, Serialize, Deserialize, Debug, Clone, PartialEq)]
#[serde(crate = "near_sdk::serde")]
pub struct TransferCallData(pub String);

impl Deref for TransferCallData {
    type Target = str;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl Display for TransferCallData {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        self.0.fmt(f)
    }
}

@luciotato
Copy link

  1. it should be as simple as possible

My 2 cents. To make it simpler to read and understand, let's remove the strongly domain typed params.
Arg names: sender_id, receiver_id, amount are more than enough to express intention

  1. NOTE: #[callback_result] is not supported yet a

Then let's return void from the call, and just check if it was promise_success or reverse the transfer. That's the simplest mechanism.

@evgenykuzyakov
Copy link
Contributor Author

NOTE: #[callback_result] is not supported yet a
Then let's return void from the call, and just check if it was promise_success or reverse the transfer. That's the simplest mechanism.

It's not supported as a high-level API, but supported on slightly lower-level using promise_success and promise_result. We'll add it as a high-level to near-sdk-rs eventually.

@oysterpack
Copy link

To make it simpler to read and understand, let's remove the strongly domain typed params.
Arg names: sender_id, receiver_id, amount are more than enough to express intention

the type system and compiler is your friend ... the more information you can provide the compiler, the more you can leverage and benefit from the compiler to help you to produce robust and high quality code. This is especially true and needed when writing smart contracts - IMHO. In addition, with Rust's zero-cost abstractions, you can model your domain in the code, but pay zero cost at runtime.

One other thing to note is that the Rust contract interface is decoupled from the JSON RPC interface. For example, both snippets of code map to the same JSON RPC API

fn ft_transfer_call(
        &mut self,
        receiver_id: ValidAccountId,
        amount: TokenAmount,
        data: TransferCallData,
        memo: Option<Memo>,
    ) -> Promise;
fn ft_transfer_call(
        &mut self,
        receiver_id: String,
        amount: String, // near_sdk provides a U128 wrapper, but on the wire its a string to work around JavaScript
        data: String,
        memo: Option<String>,
    ) -> Promise;

Personally, I rather choose to work with the strongly typed interface when writing the Rust contract - but you are free to implement the same contract interface how you wish.

@robert-zaremba
Copy link
Contributor

Change Proposal: drop msg and add optional attributes to transfer_call

I'm against dropping msg. As explained in #136 :

  • msg is required to instrument the receiver
  • memo is used for accounting purposes.

@robert-zaremba
Copy link
Contributor

Change Proposal: namespace interface function names ft_ prefix

I'm not convinced by this. Are we going to use prefixes for every smart-contract standard? This is not a Rust style, nor TypeScript nor AssemblyScript. We don't need to have namespaces in function names.
Moreover, if 2 token standard will share a same function - I would see it as a big benefit. Adding "namespace" prefixes will destroy that.

@robert-zaremba
Copy link
Contributor

I guess that depends on the purpose of the transfer_call. To me it says, transfer the tokens and then notify the receiver of the transfer. In addition, because the tokens are not locked, there is potential for race conditions. These are edge cases, but when it comes to "financial" transactions, race conditions are not acceptable.

@oysterpack There is no race condition in transfer_call: the funds are sent and the receiver can use them.
Think about it this way: it's user intention to send tokens and (s)he notifies the receiver about that. It's not receiver demanding for tokens. Also, there is no need to introduce complexity of vaults. If a user don't want to send tokens then (s)he wont' send.

@robert-zaremba
Copy link
Contributor

robert-zaremba commented Jan 12, 2021

rename msg to data to make the intent more clear. make it optional: Option

Data is even more ambiguous. We already had a chat about it, and I will strongly defend msg: it's a message for the receiver.
Optional doesn't make sense for string arguments in this case. We will only add problems: what's the difference between empty string and not defined string?

Also, re custom types (TransferCallData, Memo), or using bytes for msg:

  • We should use String for interoperability reasons. These are public functions and must be available by contracts in any language.
  • The value of msg and memo is arbitrary, so I don't see any reason to complicate and create a custom type.

@robert-zaremba
Copy link
Contributor

robert-zaremba commented Jan 12, 2021

I agree with all @evgenykuzyakov comments.
Also:

  • Would prefer to not use ft_ prefix (see my comment about it above)
  • we should require that FungibleTokenCore extends AccountRegistration trait (or whatever other name we will set for that interface).

@miohtama
Copy link

Whatever this msg format internally as long as it's a JSON string is not defined by this standard

@evgenykuzyakov Got it. I was looking the earlier discussion, got confused.

@luciotato
Copy link

luciotato commented Jan 16, 2021

It's not supported as a high-level API,

It's not supported yet as a high-level API, I would recommend to not include it in this standard. It will discourage newcomers.

We should return void from the call, and just check if it was promise_success or reverse the transfer. That's the simplest mechanism. The rare use cases where the receiving contract does not keeps all the transferred tokens can be solved by the receiving contract calling ft_transfer to return the extra tokens.

pub trait FungibleTokenCore {
    #[payable]
    fn ft_transfer(&mut self, receiver_id: AccountId, amount: U128, memo: Option<String>);

    #[payable]
    fn ft_transfer_call(
        &mut self,
        receiver_id: AccountId,
        amount: U128,
        msg: String,
        memo: Option<String>,
    ) -> Promise;

    fn ft_total_supply(&self) -> U128;

    fn ft_balance_of(&self, account_id: AccountId) -> U128;
}

pub trait FungibleTokenReceiver {
    fn ft_on_transfer(
        &mut self,
        sender_id: AccountId,
        amount: U128,
        msg: String,
        memo: Option<String>
    ) ;
}

pub trait FungibleTokenCore  {
    #[private]
    //this is executed after ReceivingContract.ft_on_transfer()...
    fn ft_resolve_transfer(
        &mut self,
        sender_id: AccountId,
        receiver_id: AccountId,
        amount: U128
    );
   // undo the transfer if the receiving contract did not execute correctly
   // { if !env::is_promise_success() { self.internal_transfer(receiver_id, sender_id, amount.0 } }
}

@evgenykuzyakov
Copy link
Contributor Author

evgenykuzyakov commented Jan 16, 2021

We should return void from the call, and just check if it was promise_success or reverse the transfer. That's the simplest mechanism. The rare use cases where the receiving contract does not keeps all the transferred tokens can be solved by the receiving contract calling ft_transfer to return the extra tokens.

When checking promise_success, it's as easy to check the result and parse it.

Also we plan to add callback_result in Q1 to near-sdk. It's very useful to have this logic built in, since it mimics allowance flexibility that is needed for dynamic pricing in async environment

@luciotato
Copy link

Ok, I understand, it is required to code slippage more efficiently in something like Uniswap V2 on NEAR native

@oysterpack
Copy link

thanks @mikedotexe for organizing the "FT Standard Discussion" meeting tomorrow. I propose we use the JSON API as the baseline to organize the discussion around.

It's been a great discussion and it would be good to summarize where people stand going into the meeting.
Please do a final review and cast your vote with a thumbs up or thumbs down on the JSON API .

  • thumbs up means you have no objections and accept the JSON API as the FT core standard.
  • if you vote thumbs down means please post a comment explaining why

@luciotato
Copy link

luciotato commented Jan 18, 2021

Proposal: Invert the returned value from the on_ft_transfer
Since is the "returned" value, the natural thing is to return the "unused amount to return"


The method MUST return the amount of unused tokens. Examples:

The transferred amount was 500, the contract completely takes it and MUST return 0.
The transferred amount was 500, but this transfer call only needs 450 for the action passed in the msg
field, then the method MUST return 50.
The transferred amount was 500, but the action in msg field has expired and the transfer should be
cancelled. The method MUST panic. A "panic" will trigger a complete refund.

Arguments:

sender_id - the account ID that initiated the transfer.
amount - the amount of tokens that were transferred to this account in a decimal string representation.
msg - a string message that was passed with this transfer call.
Returns the amount of unused tokens that should be returned to sender, in a decimal string representation.

@robert-zaremba
Copy link
Contributor

Let's change the language.

  • don't use should
  • use: MUST, MUST NOT, MAY

@oysterpack
Copy link

exactly 1 yoctoNEAR must be attached to ft_transfer and ft_transfer_call

@ilblackdragon
Copy link
Member

ilblackdragon commented Jan 23, 2021

@nearmax raised some concerned about JSON serialization: https://gov.near.org/t/evm-runtime-base-token/340/24

So I just wanted to make sure it's decided that all arguments in the standard are going to be serialized in JSON?

@robert-zaremba
Copy link
Contributor

@ilblackdragon I responded in the gov.near.org. I don't know what Node is using for wallet / dapp API - I don't think this is a problem.

In this standard, there are no complex arguments - all of them are only strings which doesn't require a complex object. So it's a string representing a number, or a string representing some not defined bytes (eg base64), or just a string (message, account ,...).

Developer may decide to use JSON to encode a complex object in msg or memo. But I also don't think it's a problem - because the contract will be compiled only once using a specific version of libraries (hence JSON parser implementation), and a binary will be distributed across the nodes. Hence it won't be susceptible for different JSON implementation when the contract is run.

@ilblackdragon
Copy link
Member

@robert-zaremba it's not a big deal indeed in the case of this standard, but protocol ideally should be very well defined on the byte level.

For example are any of these valid parsed arguments:

  • {"receiver_id": "alice", "amount": "100", "msg": "bXNzZw==", }
  • \0x13{"receiver_id": "alice", "amount": "100", "msg": "bXNzZw=="}
  • \r\n{"receiver_id": "alice",\n "amount": "100",\r "msg": "\nbXNzZw==\r"}
  • {"receiver_id": "alice", "amount": "100", "msg": "bXNzZw=="} blah blah
  • U+0000{"receiver_id": "alice", "amount": "100", "msg": "bXNzZw=="}
  • {"receiver_id": null, "amount": "100", "msg": "bXNzZw=="}

There are plenty of weird JSON stuff: https://github.com/c9fe/weird-json
and here for example set of things supported by one of the parser (and potentially not by others): https://github.com/yw662/rjson#reminder

@evgenykuzyakov
Copy link
Contributor Author

Please see the continuation of this discussion in #146

The storage management standard is discussed in #145
The fungible token metadata is discussed in #148

@robert-zaremba
Copy link
Contributor

Wrapping out and putting things together

I'm wrapping all this out in : https://github.com/defi-guild/fungible-token

IMPORTANT:

The order of arguments in fn ft_transfer_call should change: memo should go before msg. The reason for this is that in ft_transfer we don't have msg and memo is right after amount. So ft_transfer_call should have the same order.

@luciotato
Copy link

luciotato commented Feb 15, 2021

I have a few points:

  1. I believe the "returned amount" should be the "unused amount", we've already discussed this and settled on "unused amount".
    See here Fungible Token Core Standard #141 (comment)

  2. @robert-zaremba I understand your reasons for the args order change, msg and memo. But from the client-side, and in any programing language with optional parameters but not named parameters, you need to put optional args at the end. For example in TS, you declare a public function x (a:number, b:string, c?:string) meaning you can call it with 2 or 3 args, and if you call it with 2 the 3rd is "undefined"=>null=>None. So maybe we have to leave it like it was, even if the order is reversed.

  3. (related to 2) this might be a stupid question already discussed, but why not use both msg: Option<String>, memo: Option<String>, both optional and both in both calls to simplify things? why not add a msg: Option<String> to ft_transfer ? Even if there's not a clear use for msg in ft_transfer now, it maybe help expanding functionality in future iterations without changing the function signatures while making the current standard simpler.

@luciotato
Copy link

luciotato commented Feb 15, 2021

Hey @robert-zaremba you also removed all @evgenykuzyakov 's code comments in the functions. I think the comments help a lot and should be kept. It was:

    /// Simple transfer.
    /// Transfers positive `amount` of tokens from the `env::predecessor_account_id` to `receiver_id`.
    /// Both accounts should be registered with the contract for transfer to succeed.
    /// Method is required to be able to accept attached deposits - to not panic on attached deposit. See Security
    /// section of the standard.
    ///
    /// Arguments:
    /// - `receiver_id` - the account ID of the receiver.
    /// - `amount` - the amount of tokens to transfer. Should be a positive number in decimal string representation.
    /// - `memo` - an optional string field in a free form to associate a memo with this transfer.
    pub fn...

and here https://github.com/defi-guild/fungible-token the comments are removed:

    /// Simple transfer.
    pub fn... 

@jasperdg
Copy link

jasperdg commented Feb 15, 2021

Playing around with the NEP-141 standard in our current code base. Problem I'm running into is that I would prefer for users not to have to "pre-pay" storage. I think the standard still holds true but it would be better if in the transfer_and_call method we'd allow for > 1 yocto and assume that all yocto attached would be used to fund storage on the target contract.

The implementation would be more similar to this:

        ext_fungible_token_receiver::ft_on_transfer(
            sender_id.clone(),
            amount.into(),
            msg,
            receiver_id.as_ref(),
            env::attached_deposit(),
            env::prepaid_gas() - GAS_FOR_FT_TRANSFER_CALL,
        )
        .then(ext_self::ft_resolve_transfer(
            sender_id,
            receiver_id.into(),
            amount.into(),
            &env::current_account_id(),
            NO_DEPOSIT,
            GAS_FOR_RESOLVE_TRANSFER,
        ))

I think the pre-paid model is a good fallback model but that it should be optional since it does require an extra step and will throw users / frontend devs off once storage runs out.

Maybe this would cause the same issue as we're trying to solve before where a user's deposit would get "stuck" in the token contract if the promise fails but I think this could be handled in resolve_transfer

@robert-zaremba
Copy link
Contributor

@luciotato - we will add comments in the detailed documentation, I just wanted to have a quick and short overview.

As for optional - to be hones I think both of them should be String (not Option<String>) - possible empty. I don't see how someone should distinguish empty string from null.

@robert-zaremba
Copy link
Contributor

@jasperdg - good point. For discussion we should use #146

@jasperdg
Copy link

@jasperdg - good point. For discussion we should use #146

I'll add this comment to #146 then

@oysterpack
Copy link

NOTE: this NEP is closed and the new process is to use discussions (for discussions)

luciotato added a commit to luciotato/NEPs that referenced this issue Mar 6, 2021
can confuse newcomers. Eugene could also update what's at http://github.com/near/neps/pull/141 to reflect the new standard
@mfornet
Copy link
Member

mfornet commented Apr 8, 2021

NOTE: this NEP is closed and the new process is to use discussions (for discussions)

Let's add a link in the top of this issue description linking to the NEP-141 discussion. cc @evgenykuzyakov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants