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

Proposal: Allowance-free vault-based token standard #122

Closed
evgenykuzyakov opened this issue Oct 6, 2020 · 43 comments
Closed

Proposal: Allowance-free vault-based token standard #122

evgenykuzyakov opened this issue Oct 6, 2020 · 43 comments
Assignees

Comments

@evgenykuzyakov
Copy link
Contributor

evgenykuzyakov commented Oct 6, 2020

Rational:

  • remove the extra space that is used for allowance
  • remove an extra transaction that is required to transfer token to a contract (to set allowance)
  • simplify token standard usage
  • make chained transactions possible (even, though they are complicated)
  • ideally remove #[payable] requirements, because it's not easily abusable. You'd have to transfer to non existing accounts. This can be addressed with minimum token balance.

Background

There are a few reasons for allowance:

  • Allow to withdraw tokens from the owner's account by the contract at any time.
    This can be solved by depositing tokens to the contract first and it can spend the tokens on your behalf.
  • Legacy design model where in order to act on your behalf, you first authorize the contract and then it can
    withdraw from you when you initialized the transfer. This can be addressed through a callback.

Allowance is also often abused by dApps setting unlimited allowance all the time, so it defeats the purpose.

Safe-based transfers

Instead of having permanent allowance, we can introduce a one-time temporary allowance that only lives for the
duration of the transaction. We call this a safe. This idea is very similar to Auto-unlock with Safes idea, but it doesn't require protocol changes in the nearcore Runtime.

It works the following way:

  • An owner calls transfer_with_safe where the receiving side is a contract.
  • The token contract withdraws the amount of tokens from the owner and temporary locks them.
  • Then it calls a receiving contract with a unique identifier that can be used to access locked tokens.
  • The receiving contract can withdraw up to the amount of token from the temporary lock and use them.
  • Once the receiving contract call is done, the callback on the token contract is triggered and it returns the unspent tokens from the lock.

We call this temporary lock a safe.

Implementation

Token interface

/// Simple transfers
/// Gas requirement: 5 TGas or 5000000000000 Gas
/// Should be called by the balance owner.
///
/// Actions:
/// - Transfers `amount` of tokens from `predecessor_id` to `receiver_id`.
pub fn transfer_unsafe(&mut self, receiver_id: ValidAccountId, amount: U128);

/// Transfer to a contract with payload
/// Gas requirement: 40+ TGas or 40000000000000 Gas.
/// Consumes: 30 TGas and the remaining gas is passed to the `receiver_id` (at least 10 TGas)
/// Should be called by the balance owner.
/// Returns a promise, that will result in the unspent balance from the transfer `amount`.
///
/// Actions:
/// - Withdraws `amount` from the `predecessor_id` account.
/// - Creates a new local safe with a new unique `safe_id` with the following content:
///     `{sender_id: predecessor_id, amount: amount, receiver_id: receiver_id}`
/// - Saves this safe to the storage.
/// - Calls on `receiver_id` method `on_token_receive(sender_id: predecessor_id, amount, safe_id, payload)`/
/// - Attaches a self callback to this promise `resolve_safe(safe_id, sender_id)`
pub fn transfer_with_safe(
    &mut self,
    receiver_id: ValidAccountId,
    amount: U128,
    payload: String,
) -> Promise;

/// Withdraws from a given safe
/// Gas requirement: 5 TGas or 5000000000000 Gas
/// Should be called by the contract that owns a given safe.
///
/// Actions:
/// - checks that the safe with `safe_id` exists and `predecessor_id == safe.receiver_id`
/// - withdraws `amount` from the safe or panics if `safe.amount < amount`
/// - deposits `amount` on the `receiver_id`
pub fn withdraw_from_safe(
    &mut self,
    safe_id: SafeId,
    receiver_id: ValidAccountId,
    amount: U128,
);

/// Resolves a given safe
/// Gas requirement: 5 TGas or 5000000000000 Gas
/// A callback. Should be called by this fungible token contract (`current_account_id`)
/// Returns the remaining balance.
///
/// Actions:
/// - Reads safe with `safe_id`
/// - Deposits remaining `safe.amount` to `sender_id`
/// - Deletes the safe
/// - Returns the total withdrawn amount from the safe `original_amount - safe.amount`.
/// #[private]
pub fn resolve_safe(&mut self, safe_id: SafeId, sender_id: AccountId) -> U128;

Receiving side interface

/// Called when a given amount of tokens is locked in a safe by a given sender with payload.
/// Gas requirements: 2+ BASE
/// Should be called by the fungible token contract
///
/// This methods should withdraw tokens from the safe and act on them. When this method returns a value, the
/// safe will be released and the unused tokens from the safe will be returned to the sender.
/// There are bunch of options what the contract can do. E.g.
/// - Option 1: withdraw and account internally
///     - Increase inner balance by `amount` for the `sender_id` of a token contract ID `predecessor_id`.
///     - Promise call `withdraw_from_safe(safe_id, receiver_id: env::current_account_id(), amount)` to withdraw the amount to this contract
///     - Return the promise
/// - Option 2: Simple redirect to another account
///     - Promise call `withdraw_from_safe(safe_id, receiver_id: ANOTHER_ACCOUNT_ID, amount)` to withdraw to `ANOTHER_ACCOUNT_ID`
///     - Return the promise
/// - Option 3: Partial redirect to another account (e.g. with commission)
///     - Promise call `withdraw_from_safe(safe_id, receiver_id: ANOTHER_ACCOUNT_ID, amount: ANOTHER_AMOUNT)` to withdraw to `ANOTHER_ACCOUNT_ID`
///     - Chain with (using .then) promise call `withdraw_from_safe(safe_id, receiver_id: env::current_account_id(), amount: amount - ANOTHER_AMOUNT)` to withdraw to self
///     - Return the 2nd promise
/// - Option 4: redirect some of the payments and call another contract `NEW_RECEIVER_ID`
///     - Promise call `withdraw_from_safe(safe_id, receiver_id: current_account_id, amount)` to withdraw the amount to this contract
///     - Chain with promise call `transfer_with_safe(receiver_id: NEW_RECEIVER_ID, amount: SOME_AMOUNT, payload: NEW_PAYLOAD)`
///     - Chain with the promise call to this contract to handle callback (in case we want to refund).
///     - Return the callback promise.
on_receive_with_safe(sender_id: ValidAccountId, amount: U128, safe_id: SafeId, payload: String);
@miohtama
Copy link

miohtama commented Oct 7, 2020

Here I will drop some comments in random order.

on_receive_with_safe(sender_id: ValidAccountId, amount: U128, safe_id: SafeId, payload: String);

I suggest having payload asVec<u8> as it is more efficient. Most use cases with message involved determining the message type (first byte) and then parsing its content and taking a different action based on the message type. If someone wants to transfer human-readable data then they can just decode payload as UTF-8.

The following is only maybe good idea: A lot of smart contract use cases need to know the current balance (balance_of) and the incoming transfer amount. I suggest adding two amounts: amount_transferred and amount_total, because this way the receiving contract avoids the round-trip to ask "what is my current balance now". Alternatively the receving contract can also keep the total balance on itself as a counter, but it is just simpler to pass it as an argument.

@miohtama
Copy link

miohtama commented Oct 7, 2020

pub fn transfer(receiver_id: ValidAccountId, amount: U128);

I suggest trying to simplify so that there is only one entry-point for transfers:

pub fn transfer(receiver_id: ValidAccountId, amount: U128, payload: Vec<u8>);

Do not make two similar functions, as the end-users still may end up to use the wrong one. Here is an example of an end-user executing ERC-20 transfer() against a contract account:

https://etherscan.io/tx/0x18ff0886581735dc189bdd96a77f01b43a03fe769055fe25f78f3c7d2c1e5502

In this case, the contract or the user cannot recover from this situation.

Here are some different use cases we need to separate out

  • Smart contract is about to send out tokens and the receiver is non-code account

  • Smart contract is about to send out tokens and the receiver is code account, on_receiver_with_safe needs to be called

  • Smart contract is about to send out tokens and the receiver is code account, on_receiver_with_safe needs to be called, on_receiver_with_safe panics

  • User is about to send out tokens and the receiver is non-code account

  • User is about to send out tokens and the receiver is code account on_receiver_with_safe needs to be called

  • User is about to send out tokens and the receiver is code account on_receiver_with_safe needs to be called, on_receiver_with_safe panics

Ideally they all would use the same call signature, and behind the scenes the token smart contract would construct the promise in such a way that transfer_with_safe is used internally when needed. However, a new feature for NEAR VM might be needed: either by executing a promise (on_receive_with_safe) only if the target is a code account or let the smart contract query if the target is a code account.

Alternatively if the above is not possible offer a standardized function to recover from the situation when the wrong transfer was used.

@miohtama
Copy link

miohtama commented Oct 7, 2020

A good token standard needs some additional features outside the core transfer semantics

  1. A good metadata so wallets can display tokens correctly

  2. A way for a wallet to easily query all the tokens user have received, to populate the list of assets of the user

  3. Standardised mint and burn functionality

For 1) it is just coming up with some core metadata fields besides name

For 2) I have no idea how it is supposed to work on NEAR: Is there are a centralised (yuc) indexer server that all clients rely on to get this kind of data?

For 3) is now needed more in DeFi scenarios than back in the day when tokens started. Ethereum mints and burns are somwhat fragmented, so it is hard for blockchain explorers like Etherscan to parse when new tokens popped in and out of existence. This leads to the fact that often "the total available supply" number is not correctly tracked.

@evgenykuzyakov
Copy link
Contributor Author

Regarding payload: String. It's just a generic JSON entry. The receiving side can interpret it how they want. If they want to have Vec<u8>, then there is a helper class Base64VecU8 that takes input as a String and decodes Vec<u8> from base64. The reason for not using raw Vec<u8> is the JSON serialization is not efficient. It'll produce something like: [123, 123, 32, 0]. That's why we prefer to use base64 for encoding raw bytes.

@miohtama
Copy link

miohtama commented Oct 7, 2020

Are promises going to be serialised as JSON forever? Isn't that quite inefficient.

@evgenykuzyakov
Copy link
Contributor Author

Yep. It seems we're not going back to Borsh anytime soon. So I can assume it'll be json forever :)

@luciotato
Copy link

luciotato commented Oct 11, 2020

@evgenykuzyakov would you consider also adding a secondary option transfer_to_contract(receiver, amount, payload) , so contract developers can have one more option and use the most convenient according to what's required for the particular interaction?

transfer_to_contract sequence:

  • First apply the transfer moving amount tokens from predecessor_id to receiver_id account
  • Schedule a call to receiver_id.on_transfer_sent(sender, amount, payload)
  • If the call fails, undo the transfer, returning amount tokens from receiver_id to predecessor_id account
/// Direct transfer to contract
/// Gas requirement: 5 TGas or 5000000000000 Gas
/// Should be called by the balance owner.
///

/// Transfer to contract with payload
/// Gas requirement: 40+ TGas or 40000000000000 Gas.
/// Consumes: 30 TGas and the remaining gas is passed to the `receiver_id` (at least 10 TGas)
/// Should be called by the balance owner.
/// chains a then-promise, that will undo the transfer if `receiver.on_transfer_sent(payload)` promise fails
///
/// Actions:
/// - Transfer `amount` from the `predecessor_id` account to `receiver_id` contract account.
/// - Calls on `receiver_id` method `on_transfer_sent(sender_id: predecessor_id, amount, payload)`/
/// - Attaches a then-callback `on_callback_transfer_to_contract(receiver_id, amount)`
pub fn transfer_to_contract(
    &mut self,
    receiver_id: ValidAccountId,
    amount: U128,
    payload: String,
);

/// Callback to check if the receiving contract processed ok
/// Gas requirement: 5 TGas or 5000000000000 Gas
/// A callback. Should be called by this fungible token contract (`current_account_id`)
/// undo the transfer if the previous promise failed
///
/// Actions:
/// - check if is_promise_success()
/// - if the previous promise failed, undoes the transfer
/// #[private]
pub fn on_callback_transfer_to_contract(&mut self, receiver_id: AccountId, amount: U128) -> U128;

Receiving side interface

/// Called when a given amount of tokens were transferred to this contract
/// Gas requirements: 2+ BASE
/// Should be scheduled by the fungible token contract during transfer_to_contract()
///
/// This methods should act on the recent transfer. 
/// The receiving contract already has the funds accredited. 
/// This mechanism is simpler but does not have automatic partial refunds.
/// If this method aborts or panics, the callback will undo the transfer (full refund)
/// If the contract needs to do a partial refund it has to schedule a transfer to return the partial funds on the NEP122 contract
/// There are bunch of options what the receiving contract can do. E.g.
/// - Option 1: register the transfer internally:
///     - Increase inner balance by `amount` for the `sender_id` of a token contract ID `predecessor_id`.
///     ...
on_transfer_sent(sender_id: ValidAccountId, amount: U128, payload: String);

@evgenykuzyakov
Copy link
Contributor Author

transfer_to_contract sequence:

  • First apply the transfer moving amount tokens from predecessor_id to receiver_id account
  • Schedule a call to receiver.on_transfer_sent(sender,amount, payload)
  • If the call fails, undo the transfer, returning amount tokens from receiver_id to predecessor_id account

I think the issue with this solution is to be able to spend transferred tokens while they are in-flight. E.g. a contract may be able to transfer them all out and then fail (in multiple promises). The token contract will not be able to cancel the transfer.

@luciotato
Copy link

luciotato commented Oct 12, 2020

The counter-argument is that normally receiving contracts have a limit to spend tokens based on its internal accounting. So it can't spend the funds in-flight because they're not registered in its internal accounting.

@evgenykuzyakov
Copy link
Contributor Author

If feel like safe approach is more explicit. It requires a contract to withdraw the tokens from the safe and specify the amount of tokens to withdraw. It doesn't require the contract not to fail after withdrawal, so it can pass the process further without having a 100% success callback.

Also you may need to do a refund. Consider uniswap with slippage limit. Let's say you want to swap 1000 DAI for 1000 USDT. You send 1001 DAI to account for potential slippage of price and uniswap contract has to refund you for the unused DAI amount.

@miohtama
Copy link

The counter-argument is that if you trust the receiving contract/is audited, it should have a limit to spend tokens based on its internal accounting. So it can't spend the funds in-flight because they're not registered in its internal accounting.

This comment makes no sense. You are transferring tokens to a contract, so you are already trusting it.

@luciotato
Copy link

luciotato commented Oct 29, 2020

Maybe I didn't express that correctly, I'll edit for clarity

@evgenykuzyakov evgenykuzyakov changed the title Proposal: Allowance-free safe-based token standard Proposal: Allowance-free vault-based token standard Nov 13, 2020
@evgenykuzyakov
Copy link
Contributor Author

FYI. I'm going to update this standard method names and topics to use vault instead of safe to avoid confusion with safe/unsafe

@robert-zaremba
Copy link
Contributor

transfer_to_contract sequence:

* First apply the transfer moving amount tokens from predecessor_id to receiver_id account

* Schedule a call to **receiver.on_transfer_sent(sender,amount, payload)**

* If the call fails, undo the transfer, returning amount tokens from receiver_id to predecessor_id account

I think the issue with this solution is to be able to spend transferred tokens while they are in-flight. E.g. a contract may be able to transfer them all out and then fail (in multiple promises). The token contract will not be able to cancel the transfer.
The

In fact, the transfer_to_contract is what we do in NEARswap (we call it transfer_sc). For safety, we can firstly call the callback, and the transfer.
I need to write that spec andpropose it for NEP.

@vgrichina vgrichina pinned this issue Dec 2, 2020
@vgrichina vgrichina unpinned this issue Dec 2, 2020
@vgrichina vgrichina pinned this issue Dec 2, 2020
@jasperdg
Copy link

jasperdg commented Dec 9, 2020

I'm trying wrap my head around gas sensitivity when there are multiple cross contract calls.

What if the on_receive_with_safe external tx runs out of gas? Wouldn't the funds be stuck? The receiver contract could have some sort of claim_safe_and_callback. A cleaner solution might be if there could be a timelock on the vault that would allow the sender to withdraw the funds after T.

@oysterpack
Copy link

I'm trying wrap my head around gas sensitivity when there are multiple cross contract calls.

What if the on_receive_with_safe external tx runs out of gas? Wouldn't the funds be stuck? The receiver contract could have some sort of claim_safe_and_callback. A cleaner solution might be if there could be a timelock on the vault that would allow the sender to withdraw the funds after T.

The funds will not get stuck because the resolve_vault callback always gets called when on_receive_with_safe completes.

@oysterpack
Copy link

The proposed spec has evolved based on @evgenykuzyakov latest and greatest "reference implementation" :

My feedback on the latest and greatest is:

  • rename the simple transfer_raw to simply transfer
  • change VaultId type from u64 to U128: pub struct VaultId(pub U128)
    • number needs to be serialized as JSON string because of JSON number limitation
    • u64 should be big enough as a sequence based ID, but u128 is even bigger (and more robust)
  • remove the payable constraint on the transfer and transfer_with_vault - instead accounts must be registered
    in order to transact tokens, i.e., transfer and withdraw tokens
    • both parties must be registered, i.e., the token sender and receiver

Registered Accounts

  • when an account registers, it is required to pay for its own account storage usage - the storage fees are escrowed and will be refunded when the account unregisters with the contract
  • what happens if an account has a non-zero token balance and wants to unregister, i.e., "close", the account?
    • the answer is, it depends ... on the contract business rules - e.g.,
      • the contract may allow the account to burn its tokens when unregistering (and is refunded its NEAR storage escrowed fees)
      • the contract may not allow the account to unregister with a zon-zero balance. The account must first transfer all tokens out of its account before it is allowed to unregister.

Thoughts on "How do we finalize the standard ?"

... this is a great discussion but we need to bring this to closure soon in order to drive adoption .... To move forward, I propose having live online discussions (via Zoom or other venue) and design sessions to hammer this out. Can someone from the NEAR DEV team please drive this and set this up? I vote for @evgenykuzyakov :)

  • as an FYI, I am just starting on implementing a vault-based FT on the contract I am working on applying the changes I have outlined above, but in the end I want to implement the agreed upon standard.

... closing thoughts ... we need to work on standardizing the process to standardize

@robert-zaremba
Copy link
Contributor

robert-zaremba commented Dec 22, 2020

@oysterpack - I agree - we need to finalize standards to drive the adoption. And there is a need for "reactive" fungible tokens.
At the same time we need to make sure that we find a right and robust interface.
Please have a look at the NEP-136 Interactive Fungible token - an alternative approach for reactive FT. I like your idea with account registration. I'm happy to accommodate it there.

@oysterpack
Copy link

oysterpack commented Dec 23, 2020

Here's my two cents ... all of the proposed standards have different token "transfer" use cases. We need to decouple the transfer protocol from the Fungible Token. I whipped together some quick and dirty interfaces to illustrate the concept.

Account registration is required, especially on NEAR because of storage usage fees. The benefits for registered accounts are:

  • helps protect against "denial of storage" style attacks
  • helps to verify valid accounts and protect against tokens from being locked up forever, e.g., when using a simple direct transfer protocol, it's inevitable that users will mistype account IDs

The key to decoupling the transfer protocol interface is to let the registered account choose the transfer protocol from the receiver side. Sender's simply want to transfer tokens to the receiver, unaware of the underlying transfer protocol.

The tricky part is gas, because different protocols will have different gas profiles and requirements. Thus, the gas requirements need to be specified as part of the spec on the transfer protocol.

Thoughts?

https://github.com/oysterpack/oysterpack-near-stake-token/blob/main/contract/tests/fungible_token.rs

use near_sdk::json_types::{ValidAccountId, U128};
use near_sdk::{
    ext_contract,
    serde::{Deserialize, Serialize},
    AccountId, Promise, PromiseOrValue,
};

/// The design intent is to decouple the token asset from the token transfer protocol.
///
/// - Fungible token supports 1 or more [TransferProtocol]s as specified per [MetaData]
/// - Accounts must register with the token contract and pay for account storage fees.
///   - account storage fees are escrowed and refunded when the account unregisters
///   - account chooses the transfer protocol to use as transfer recipient
/// - FT has generic [transfer] function interface
/// - sender account does not choose the transfer protocol - the receiver account chooses how they
///   want to receive the tokens
///
/// The key advantage of this design is that it decouples the protocol interface from the implementation.
/// The problem with all of the "standard" interfaces is that they are too tightly coupled with implementation.
/// We need decoupled interface that will allow transfer protocols to evolve.
pub trait FungibleToken: AccountRegistry {
    fn metadata() -> Metadata;

    /// Returns total supply.
    /// MUST equal to total_amount_of_token_minted - total_amount_of_token_burned
    fn total_supply(&self) -> U128;

    /// Returns the token balance for `holder` account
    fn balance_of(&self, account_id: ValidAccountId) -> U128;

    /// ## Panics
    /// - if accounts are not registered
    /// - insufficient funds
    fn transfer(
        &mut self,
        receiver_id: ValidAccountId,
        amount: U128,
        msg: Option<String>,
        memo: Option<String>,
    ) -> PromiseOrValue<TransferProtocol>;
}

/// Suggested protocol names:
/// - NEP_122
/// - NEP_136
/// - NEP_21
///
/// - each protocol defines min amount of gas required excluding gas required to cover `msg` `memo`
pub struct TransferProtocol(String, Gas);

pub struct Gas(pub u64);

pub trait AccountRegistry {
    /// Registers the predecessor account ID with the contract.
    /// The account is required to pay for its storage. Storage fees will be escrowed and refunded
    /// when the account is unregistered.
    ///
    /// #[payable]
    /// - storage escrow fee is required
    ///   - use [account_storage_escrow_fee] to lookup the required storage fee amount
    /// - any amount above the storage fee will be refunded
    ///
    /// ## Panics
    /// - if deposit is not enough to cover storage fees
    /// - is account is already registered
    /// - if transfer protocol is not supported
    ///
    /// #[payable]
    fn register_account(&mut self, transfer_protocol: TransferProtocol);

    /// Unregisters the account and refunds the escrowed storage fees.
    ///
    /// ## Panics
    /// - if account is not registered
    /// - if registered account has funds
    fn unregister_account(&mut self);

    /// changes the account's transfer type
    ///
    /// ## Panics
    /// - if account is not registered
    /// - if transfer protocol is not supported
    fn set_transfer_type(&mut self, transfer_protocol: TransferProtocol);

    /// Burns owned token funds unregisters the account. Escrowed storage fees are refunded.
    ///
    /// ## Panics
    /// - if account is not registered
    fn burn_account(&mut self);

    ////////////////////////////
    ///     VIEW METHODS    ///
    //////////////////////////

    /// Returns the required deposit amount that is required for account registration.
    fn account_storage_fee(&self) -> U128;

    fn account_registered(&self, account_id: ValidAccountId) -> bool;

    /// returns None if the account is not registered
    fn account_transfer_protocol(&self, account_id: ValidAccountId) -> Option<String>;

    fn total_registered_accounts(&self) -> U128;
}

/// Each token must have 18 digits precision (decimals)
pub const DECIMALS: u8 = 18;

pub struct Metadata {
    pub name: String,
    pub symbol: String,

    /// URL to additional resources about the token.
    pub reference: String,

    /// the smallest part of the token that’s (denominated in e18) not divisible
    /// In other words, the granularity is the smallest amount of tokens (in the internal denomination)
    /// which MAY be minted, sent or burned at any time.
    /// - The following rules MUST be applied regarding the granularity:
    /// - The granularity value MUST be set at creation time.
    /// - The granularity value MUST NOT be changed, ever.
    /// - The granularity value MUST be greater than or equal to 1.
    /// - All balances MUST be a multiple of the granularity.
    /// - Any amount of tokens (in the internal denomination) minted, sent or burned MUST be a
    ///   multiple of the granularity value.
    /// - Any operation that would result in a balance that’s not a multiple of the granularity value
    ///   MUST be considered invalid, and the transaction MUST revert.
    ///
    /// NOTE: Most tokens SHOULD be fully partition-able. I.e., this function SHOULD return 1 unless
    ///       there is a good reason for not allowing any fraction of the token.
    pub granularity: U128,

    /// Transfer protocols that are supported by the token contract
    pub supported_transfer_protocols: Vec<TransferProtocol>,
}

@oysterpack
Copy link

To help drive this, we should pull in the NEAR wallet team into the discussion ... the NEAR wallet should/must have built in support for FT and would be an excellent POC to drive the FT interface standardization. If we can make this work for the NEAR wallet, then it would pave the way for community adoption and be a boon for the NEAR ecosystem.

@evgenykuzyakov
Copy link
Contributor Author

evgenykuzyakov commented Dec 23, 2020

I agree that the account registration should be a separate standard. Because this pattern is likely will be used for other contracts and standards.

As for the fungible token standard, I tried to design the bare minimum that is required for the transfers to work.

Metadata is not part of the standard, because it's not needed for the main functionality to work. It's required for wallets to display the token, but it doesn't have to belong to this particular standard.

As for the memo, it's an interesting question. Since we use JSON, we can include it as an optional field, that will be automatically ignored by the contract that do not want to work with memo.

Also, note that this standard lacked balance_of view method, which probably should be included into this standard.

@robert-zaremba
Copy link
Contributor

Here's my two cents ... all of the proposed standards have different token "transfer" use cases. [...]

It this a comment to this proposal, or to #136 ?

@robert-zaremba
Copy link
Contributor

I was writing about a need for memo in last summer, and included it the first version of NEARswap (for the multi-tokens). I don't think we should use Option<> in public API though. It complicates. Most of the types should be simple. Option could be used for an optional numeric value.

@robert-zaremba
Copy link
Contributor

@oysterpack , your motivation for AccountRegistry is to separate the accounting from storage, right? I don't think it will work here, because token implementations may have different storage layout (that being said, I believe most of them will be simple and have the same layout). Moreover separating AccountRegistry from Token into 2 different contracts will complicate implementation - each transfer will require additional async call.

@evgenykuzyakov
Copy link
Contributor Author

I was writing about a need for memo in last summer, and included it the first version of NEARswap (for the multi-tokens). I don't think we should use Option<> in public API though. It complicates. Most of the types should be simple. Option could be used for an optional numeric value.

memo: Option<String> in Rust will be handled fairly easy. It's either there in json like {"memo": "ABC"} or it's just not there {} or {"memo": null}. Having it as required for something the contract doesn't use is waste of gas, similar how having a full metadata in the contract is not required. Just a verifiable link to get it.

@evgenykuzyakov
Copy link
Contributor Author

But if we standardize events as part of this NEP and one of the event requires to put a memo, then we can include it as required. But even an event may have optional fields.

@robert-zaremba
Copy link
Contributor

@evgenykuzyakov is an empty string ("") more expensive than null? If so, is it "noticeable" to the contract call?

@oysterpack
Copy link

@robert-zaremba I am not saying to have a separate contract for AccountRegistry - I am saying the token contract should implement the AccountRegistry interface in addition to the FungibleToken interface:

pub trait FungibleToken: AccountRegistry {
...
}

and regarding Option ... it is a best practice because the intent is explicit, i.e., something that is optional should be wrapped in Option

@robert-zaremba
Copy link
Contributor

robert-zaremba commented Dec 24, 2020

Ah, I didn't notice the inheritance.
Re Option: how do you distinguish None from Some("") (in the business logic)? I see a reason for it for numeric arguments, or structural arguments.

@oysterpack
Copy link

oysterpack commented Dec 24, 2020

@robert-zaremba

Re Option: how do you distinguish None from Some("") (in the business logic)? I see a reason for it for numeric arguments, or structural arguments.

Required numeric arguments should never be passed in as empty string, and it is better to fail fast with a validation error. The intent should be clear ... let the type system do its job - it makes the code cleaner, more robust, prevents bugs, helps protect against human error, etc

That being said, let's say you had reason for a type such as Option<String>, then you would simply use a match clause to distinguish None from Some(""):

   let value: Option<String> = Some("".to_string());
    match value {
        None => println!("value is None"),
        Some(value) if value.is_empty() => println!("value is empty string"),
        Some(value) => println!("value is {}", value),
    }

@oysterpack
Copy link

I thought it would be good to share some working code ... I have implemented NEP-122 for the STAKE token project I am working on using @evgenykuzyakov's implementation within Berry Farm as a reference with the following modifications:

  1. all token owners must be registered with the contract, which implies that token transfers can only be between registered accounts
    • this removes the need to require an attached deposit on each transfer because the accounts are pre-registered
    • the STAKE token contract requires pre-registration because it collects account storage usage fees upfront ... there is no free storage (the storage fees are escrowed and refunded to the account when it unregisters)
    • eliminates transfers to non-existent accounts
  2. renamed transfer_raw to transfer

Code

VaultFungibleToken

AccountManagement

The contract is deployed on testnet at stake.oysterpack.testnet

NEAR CLI Examples

# VaultFungibleToken
near view stake.oysterpack.testnet get_total_supply
near view stake.oysterpack.testnet get_balance --args '{"account_id":"alfio-zappala-oysterpack.testnet"}'
near call stake.oysterpack.testnet transfer --accountId alfio-zappala-oysterpack.testnet --args '{"receiver_id":"oysterpack.testnet", "amount":"1000000000000"}'

# AccountManagement
# returns the account storage fee amount that is required when registering an account
near view stake.oysterpack.testnet account_storage_fee
# payment is required for account storage usage fee (currently 0.0681 NEAR) - change is refunded
near call stake.oysterpack.testnet register_account --accountId oysterpack.testnet --amount 1
near call stake.oysterpack.testnet unregister_account --accountId alfio-zappala-oysterpack.testnet

NOTE: I haven't tested the transfer_with_vault on testnet yet ...

Feedback is welcomed and appreciated.

@robert-zaremba
Copy link
Contributor

Required numeric arguments should never be passed in as empty string, and it is better to fail fast with a validation error.

@oysterpack, yes, this is what I meant.

@robert-zaremba
Copy link
Contributor

@oysterpack - in your use-case, what is the advantage of using vaults vs #136 ?

@oysterpack
Copy link

oysterpack commented Dec 28, 2020

@robert-zaremba I would say it depends on the specific use case which is determined by the specific receiver.

I chose to implement the vault based implementation simply because I needed to choose something for my STAKE token project
(and even my implementation is a variant because of the requirement for registered accounts) ...

the use cases for both transfer_with_call and transfer_with_vault is similar but different ...
Both are designed to transact with another contract through a predefined protocol, i.e., the receiver must implement a
specific function interface.

  • NEP-122 transfer_with_vault is designed to support one-time temporary allowance.
  • NEP-126 transfer_call is deigned to execute the transfer and then notify the recipient of the transfer.

Both require a "finalize" callback on the token contract, in NEP-122 this is defined as

fn resolve_vault(&mut self, vault_id: VaultId, sender_id: AccountId) -> U128;

and in NEP-136 the proposed "finalize" callback is finalize_token_call()

NEP-122 locks up a "temporary one-time allowance" within the "vault", and it is up to the receiver to withdraw the tokens from the vault.
NEP-136 transfer_call is a specialization of NEP-122 transfer_with_vault where the receiver would always withdraw all
tokens from the vault. Thus, NEP-122 vault transfers covers the transfer_call use case, but with more overhead, i.e.,
NEP-136 is more efficient for the use case where no allowance is required and all tokens are transferred before notifying
the receiver about the token transfer.

... this brings me back to my previous comment in the discussion ...

There is a use case for both ... and there might be more use cases we think of in the future ... the problem with the current
proposed fungible token standards is that the interfaces are too tightly coupled with the underlying transfer protocol.
We need a generic FT transfer interface that is decoupled from the multitude of token transfer use cases. We have been
discussing the following token transfer use cases:

  1. NEP-21 simple transfer
  2. NEP-21 long term allowance support
    • NOTE: to prevent abuse, it must require storage usage fee to setup the allowance
  3. NEP-122 transfer with vault (one-time temporary allowance)
  4. NEP-136 reactive transfer

When the receiver is a contract, it should decide how to receive the tokens. If the token contract supports account registration
then the receiver contract can specify as part of its "preferences" how it wants to receive tokens.

Go back and take a look at: https://github.com/oysterpack/oysterpack-near-stake-token/blob/main/contract/tests/fungible_token.rs

This discussion has been focused on the token transfer protocol, but standardizing metadata and having a token registry are
also fundamental ... we need to have that discussion, but let's first continue to focus on the token transfer interface.

In closing, I leave with you with some food for thought ...

How can we bring this FT standard home and to a conclusion ?

"the proof is in the pudding" ... we need to drive and prove the standard with live code. I offer my STAKE token project to be used as a POC, but that is only side of the equation. We need to prove is end-to-end. We also need POC contracts that represent receiver accounts. And most importantly, to prove the usability and complete experience we need an FT wallet POC application. The logical best choice that comes to mind is the NEAR wallet. Including standard FT support in the NEAR wallet would be huge and pave the way for an illustrious FT market. This is key to deliver on NEAR's vision:

  • "The Builder’s fastest path to market"
  • "Build on Blockchain like never before"
  • "Harness the Internet of Value" - Now, everything on the internet can take on the properties of money.

To help drive this standard and bring it home, it needs leadership and some form of dedicated "task-force". We need to avoid the analysis-paralysis trap ... this is where we need to get folks from the NEAR team more engaged

@robert-zaremba
Copy link
Contributor

@oysterpack I like that.
However I don't expect that something as NEAR Wallet will support many standards. For the moment only NEP-21 is finalized.

I can use your STAKE code for NEP-136 PoC unless you will like to do it.

@oysterpack
Copy link

@robert-zaremba since I am actively working on it, I can whip that together pretty fast. I'll let you know once it's deployed on testnet ...

regarding NEAR wallet ... FT should be a core NEAR standard ... in order to harness the Internet of value, then the value must be easy to trade and transfer ... the path to fastest adoption is wallet adoption ... thus, regardless, we need to ensure the FT standard makes it simple for wallet integration and the end consumer. Having standard FT support within the NEAR wallet would be a game changer and provide a turbo boost for building dApps ... IMHO

@oysterpack
Copy link

oysterpack commented Dec 29, 2020

@robert-zaremba the latest and greatest version of the STAKE token has been deployed to testnet at stake.oysterpack.testnet

It implements 3 types of FT transfer protocols:

  1. simple transfer (NEP-21)
  2. vault based transfer (NEP-122)
  3. transfer call (NEP-136)- I named the interface

see the code and it's also included below

NOTES

  • I needed to align function signatures to be consistent and avoid collisions across all 3 transfer protocols.
  • accounts must be pre-registered with the contract
    • near call stake.oysterpack.testnet register_account --accountId oysterpack.testnet --amount 1
  • transfers are not payable - because accounts are pre-registered
  • metadata
    • does not have decimals field because it is const and must be 18
    • added field named supported_transfer_protocols - which includes gas requirements
  • the code is a quick prototype - needs more thorough testing
    • TODO: deploy test harness contracts to test the vault based and transfer call token transfers
  • added msg and memo fields to all transfer funcs as optional
use crate::domain::{self, Gas};
use near_sdk::json_types::{ValidAccountId, U128};
#[allow(unused_imports)]
use near_sdk::AccountId;
use near_sdk::{
    borsh::{self, BorshDeserialize, BorshSerialize},
    ext_contract,
    serde::{Deserialize, Serialize},
    Promise,
};

/// - Fungible token supports 1 or more [TransferProtocol]s as specified per [MetaData]
/// - Accounts must register with the token contract and pay for account storage fees.
///   - account storage fees are escrowed and refunded when the account unregisters
///   - account chooses the transfer protocol to use as transfer recipient
pub trait FungibleToken {
    fn metadata(&self) -> Metadata;

    /// Returns total supply.
    /// MUST equal to total_amount_of_token_minted - total_amount_of_token_burned
    fn total_supply(&self) -> U128;

    /// Returns the token balance for `holder` account
    fn balance(&self, account_id: ValidAccountId) -> U128;
}

#[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(crate = "near_sdk::serde")]
pub struct Metadata {
    pub name: String,
    pub symbol: String,

    /// URL to additional resources about the token.
    pub reference: Option<String>,

    /// the smallest part of the token that’s (denominated in e18) not divisible
    /// In other words, the granularity is the smallest amount of tokens (in the internal denomination)
    /// which MAY be minted, sent or burned at any time.
    /// - The following rules MUST be applied regarding the granularity:
    /// - The granularity value MUST be set at creation time.
    /// - The granularity value MUST NOT be changed, ever.
    /// - The granularity value MUST be greater than or equal to 1.
    /// - All balances MUST be a multiple of the granularity.
    /// - Any amount of tokens (in the internal denomination) minted, sent or burned MUST be a
    ///   multiple of the granularity value.
    /// - Any operation that would result in a balance that’s not a multiple of the granularity value
    ///   MUST be considered invalid, and the transaction MUST revert.
    ///
    /// NOTE: Most tokens SHOULD be fully partition-able. I.e., this function SHOULD return 1 unless
    ///       there is a good reason for not allowing any fraction of the token.
    pub granularity: u8,

    /// Transfer protocols that are supported by the token contract
    pub supported_transfer_protocols: Vec<TransferProtocol>,
}

impl Metadata {
    /// Each token must have 18 digits precision (decimals)
    pub const DECIMALS: u8 = 18;
}

#[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(crate = "near_sdk::serde")]
pub struct TransferProtocol {
    /// Suggested protocol names:
    /// - simple - NEP-21
    /// - allowance - NEP 21
    /// - vault_transfer - NEP-122
    /// - transfer_and_notify - NEP-136
    pub name: String,
    /// - each protocol defines min amount of gas required, excluding gas required to cover `msg` `memo`
    pub gas: Gas,
}

impl TransferProtocol {
    pub fn simple(gas: Gas) -> Self {
        Self {
            name: "simple".to_string(),
            gas,
        }
    }

    pub fn allowance(gas: Gas) -> Self {
        Self {
            name: "allowance".to_string(),
            gas,
        }
    }

    pub fn vault_transfer(gas: Gas) -> Self {
        Self {
            name: "vault_transfer".to_string(),
            gas,
        }
    }

    pub fn transfer_and_notify(gas: Gas) -> Self {
        Self {
            name: "transfer_and_notify".to_string(),
            gas,
        }
    }
}

pub trait SimpleTransfer {
    /// Simple direct transfers between registered accounts.
    ///
    /// Gas requirement: 5 TGas
    /// Should be called by the balance owner.
    /// Requires that the sender and the receiver accounts be registered.
    ///
    /// Actions:
    /// - Transfers `amount` of tokens from `predecessor_id` to `recipient`.
    ///
    /// ## Panics
    /// - if predecessor account is not registered - sender account
    /// - if [recipient] account is not registered
    /// - if sender account is same as receiver account
    /// - if account balance has insufficient funds for transfer
    fn transfer(
        &mut self,
        recipient: ValidAccountId,
        amount: U128,
        msg: Option<String>,
        memo: Option<String>,
    );
}

pub trait TransferCall {
    /// Transfer `amount` of tokens from the predecessor account to a `recipient` contract.
    /// The recipient contract MUST implement [TransferCallRecipient] interface. The tokens are
    /// deposited but locked in the recipient account until the transfer has been confirmed by the
    /// recipient contract and then finalized. The transfer workflow steps are:
    /// 1. sender initiates the transfer via `transder_call`
    /// 2. token transfers the funds from the sender's account to the recipient's account but locks
    ///    the transfer amount on the recipient account. The locked tokens cannot be used until
    ///    the recipient contract confirms the transfer.
    /// 3. The recipient contract is then notified of the transfer via [`TransferCallRecipient::on_ft_receive`].
    /// 4. Once the transfer notification call completes, then the [`TransferCallRecipient::on_ft_receive`]
    ///    callback on the token contract is invoked to finalize the transfer. If the recipient contract
    ///    successfully completed the transfer notification call, then the funds are unlocked
    ///    via the [`FinalizeTransferCallback::finalize_ft_transfer`] callback. If the [`TransferCallRecipient::on_ft_receive`]
    ///    call fails for any reason, then the fund transfer is rolled back in the finalize callback.
    ///
    /// `msg`: is a message sent to the recipient. It might be used to send additional call
    //      instructions.
    /// `memo`: arbitrary data with no specified format used to link the transaction with an
    ///     external event. If referencing a binary data, it should use base64 serialization.
    ///
    /// ## Panics
    /// - if accounts are not registered
    /// - insufficient funds
    fn transfer_call(
        &mut self,
        recipient: ValidAccountId,
        amount: U128,
        msg: Option<String>,
        memo: Option<String>,
    ) -> Promise;
}

pub trait FinalizeTransferCallback {
    /// Finalizes the token transfer
    ///
    /// Actions:
    /// - if the call `TransferCallRecipient::on_ft_receive` succeeds, then commit the transfer,
    ///   i.e., unlock the balance on the recipient account
    /// - else rollback the transfer by returning the locked balance to the sender
    ///
    /// #[private]
    fn finalize_ft_transfer(&mut self, sender: AccountId, recipient: AccountId, amount: U128);
}

/// Interface for recipient call on fungible-token transfers.
/// `token` is an account address of the token  - a smart-contract defining the token
///     being transferred.
/// `from` is an address of a previous holder of the tokens being sent
#[ext_contract(ext_transfer_call_recipient)]
pub trait TransferCallRecipient {
    fn on_ft_receive(
        &mut self,
        from: ValidAccountId,
        amount: U128,
        msg: Option<String>,
        memo: Option<String>,
    );
}

#[ext_contract(ext_self_finalize_transfer_callback)]
pub trait ExtFinalizeTransferCallback {
    /// Finalizes the token transfer
    ///
    /// Actions:
    /// - if the call `TransferCallRecipient::on_ft_receive` succeeds, then commit the transfer,
    ///   i.e., unlock the balance on the recipient account
    /// - else rollback the transfer by returning the locked balance to the sender
    ///
    /// #[private]
    fn finalize_ft_transfer(&mut self, sender: AccountId, recipient: AccountId, amount: U128);
}

/// Implements [NEP-122 vault based fungible token standard](https://github.com/near/NEPs/issues/122)
/// with the following modifications:
/// - all token owners must be registered with the contract, which implies that token transfers can
///   only be between registered accounts
///   - this removes the need to require an attached deposit on each transfer because the accounts
///     are pre-registered
///   - eliminates transfers to non-existent accounts
/// - `transfer_raw` has been moved to [`SimpleTransfer::transfer`]
/// - `payload` has been replaced with `msg` and `memo` optional args
pub trait VaultBasedTransfer {
    /// Transfer to a contract with payload
    /// Gas requirement: 40+ TGas or 40000000000000 Gas.
    /// Consumes: 30 TGas and the remaining gas is passed to the `recipient` (at least 10 TGas)
    /// Should be called by the balance owner.
    /// Returns a promise, that will result in the unspent balance from the transfer `amount`.
    ///
    /// Actions:
    /// - Withdraws `amount` from the `predecessor_id` account.
    /// - Creates a new local safe with a new unique `safe_id` with the following content:
    ///     `{sender_id: predecessor_id, amount: amount, recipient: recipient}`
    /// - Saves this safe to the storage.
    /// - Calls on `recipient` method `on_token_receive(sender_id: predecessor_id, amount, safe_id, payload)`/
    /// - Attaches a self callback to this promise `resolve_safe(safe_id, sender_id)`
    ///
    /// ## Panics
    /// - if predecessor account is not registered
    /// - if [recipient] account is not registered
    /// - if sender account is same as receiver account
    /// - if account balance has insufficient funds for transfer
    fn transfer_with_vault(
        &mut self,
        recipient: ValidAccountId,
        amount: U128,
        msg: Option<String>,
        memo: Option<String>,
    ) -> Promise;

    /// Withdraws from a given vault and transfers the funds to the specified receiver account ID.
    ///
    /// Gas requirement: 5 TGas
    /// Should be called by the contract that owns a given safe.
    ///
    /// Actions:
    /// - checks that the safe with `vault_id` exists and `predecessor_id == vault.recipient`
    /// - withdraws `amount` from the vault or panics if `vault.amount < amount`
    /// - deposits `amount` on the `recipient`
    ///
    /// ## panics
    /// - if predecessor account is not registered
    /// - if predecessor account does not own the vault
    /// - if [recipient] account is not registered
    /// - if vault balance has insufficient funds for transfer
    fn withdraw_from_vault(&mut self, vault_id: VaultId, recipient: ValidAccountId, amount: U128);
}

/// implements required callbacks defined in [ExtResolveVaultCallback]
pub trait ResolveVaultCallback {
    /// Resolves a given vault, i.e., transfers any remaining vault balance to the sender account
    /// and then deletes the vault. Returns the vault remaining balance.
    ///
    /// Gas requirement: 5 TGas
    ///
    /// Actions:
    /// - Reads safe with `safe_id`
    /// - Deposits remaining `safe.amount` to `sender_id`
    /// - Deletes the safe
    /// - Returns the total withdrawn amount from the safe `original_amount - safe.amount`.
    /// #\[private\]
    ///
    /// ## Panics
    /// - if not called by self as callback
    /// - following panics should never happen (if they do, then there is a bug in the code)
    ///   - if the sender account is not registered
    ///   - if the vault does not exist
    fn resolve_vault(&mut self, vault_id: VaultId, sender_id: AccountId) -> U128;
}

/// Must be implemented by contracts that support [VaultBasedTransfer] token transfers
#[ext_contract(ext_token_receiver)]
pub trait ExtTokenVaultReceiver {
    /// Called when a given amount of tokens is locked in a safe by a given sender with payload.
    /// Gas requirements: 2+ BASE
    /// Should be called by the fungible token contract
    ///
    /// This methods should withdraw tokens from the safe and act on them. When this method returns a value, the
    /// safe will be released and the unused tokens from the safe will be returned to the sender.
    /// There are bunch of options what the contract can do. E.g.
    /// - Option 1: withdraw and account internally
    ///     - Increase inner balance by `amount` for the `sender_id` of a token contract ID `predecessor_id`.
    ///     - Promise call `withdraw_from_safe(safe_id, recipient: env::current_account_id(), amount)` to withdraw the amount to this contract
    ///     - Return the promise
    /// - Option 2: Simple redirect to another account
    ///     - Promise call `withdraw_from_safe(safe_id, recipient: ANOTHER_ACCOUNT_ID, amount)` to withdraw to `ANOTHER_ACCOUNT_ID`
    ///     - Return the promise
    /// - Option 3: Partial redirect to another account (e.g. with commission)
    ///     - Promise call `withdraw_from_safe(safe_id, recipient: ANOTHER_ACCOUNT_ID, amount: ANOTHER_AMOUNT)` to withdraw to `ANOTHER_ACCOUNT_ID`
    ///     - Chain with (using .then) promise call `withdraw_from_safe(safe_id, recipient: env::current_account_id(), amount: amount - ANOTHER_AMOUNT)` to withdraw to self
    ///     - Return the 2nd promise
    /// - Option 4: redirect some of the payments and call another contract `NEW_RECEIVER_ID`
    ///     - Promise call `withdraw_from_safe(safe_id, recipient: current_account_id, amount)` to withdraw the amount to this contract
    ///     - Chain with promise call `transfer_with_safe(recipient: recipient, amount: SOME_AMOUNT, payload: NEW_PAYLOAD)`
    ///     - Chain with the promise call to this contract to handle callback (in case we want to refund).
    ///     - Return the callback promise.
    fn on_receive_with_vault(
        &mut self,
        sender_id: AccountId,
        amount: U128,
        vault_id: VaultId,
        msg: Option<String>,
        memo: Option<String>,
    );
}

#[ext_contract(ext_self_resolve_vault_callback)]
pub trait ExtResolveVaultCallback {
    /// Resolves a given vault - transfers vault remoining balance back to sender account and deletes
    /// the vault.
    ///
    /// Gas requirement: 5 TGas or 5000000000000 Gas
    /// A callback. Should be called by this fungible token contract (`current_account_id`)
    /// Returns the remaining balance.
    ///
    /// Actions:
    /// - Reads safe with `safe_id`
    /// - Deposits remaining `safe.amount` to `sender_id`
    /// - Deletes the safe
    /// - Returns the total withdrawn amount from the safe `original_amount - safe.amount`.
    /// #[private]
    fn resolve_vault(&mut self, vault_id: VaultId, sender_id: AccountId) -> U128;
}

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

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

impl From<domain::VaultId> for VaultId {
    fn from(id: domain::VaultId) -> Self {
        Self(id.0.into())
    }
}

@evgenykuzyakov
Copy link
Contributor Author

evgenykuzyakov commented Jan 5, 2021

While vault-based token provides clean rollbacks, e.g. the receiver can't overspend the received amount, I'd propose to adapt a slightly modified NEP-136 like standard for transfer_call because of its simplicity. See my comment there: #136 (comment).

Idea is to have the following interface:

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

It will do the following:

  1. Assert at least 1 yoctoNEAR is attached to a call, to prevent function-call permission access-key security flaw. See attached comment above.
  2. assert the receiver_id account is registered (storage is allocated).
  3. take amount from the predecessor_account_id.
  4. add amount to the receiver_id account balance.
  5. call the receiver_id contract with the on_ft_receive method and pass the payload
  6. attach a callback resolve_transfer to the on_ft_receive call
  7. return the callback
pub trait TransferCallReciever {
    fn on_ft_receive(&mut self, sender_id: AccountId, amount: U128, payload: String) -> U128;
}

The receiver contract should return used_amount - the amount of tokens the receiving contract keeps from the sender's transfer amount. If the promise fails or the returned amount is not valid, it's the same as used_amount == 0.

Now the resolve_transfer method should refund to sender_id the amount amount - used_amount from the receiver_id account. If the receiver_id account doesn't have the full balance (malicious or a contract with a bug), it returns the remaining available balance.

It has the following pros and cons comparing to vault transfer:
PROS:

  • Simpler implementation for both, the token and the receiver.
  • It doesn't require to keep the logic for temporary vaults.
  • It still allows to withdraw only part of the transfer amount, so uniswap's partial withdrawal will work.
  • It requires less gas on the receiver's side (don't need to call withdraw from vault)

CONS:

  • The refund may fail, but it only the case for malicious or buggy receiver's contract.
  • It's not possible to withdraw to someone else from vault. But it's doable through a separate call.

Token metadata, account registration, and balance view calls can be discussed separately.

@robert-zaremba
Copy link
Contributor

@evgenykuzyakov are you planning to add transfer_call to NEP-122? If not then let's continue the discussion in #136 .

Re CONS:

  • a buggy or malicious contract can do bad things and we can't protect against it.
  • "It's not possible to withdraw to someone else from vault" - I think it's a feature. When I send some tokens to an entity, then that entity is responsible what to do next with the tokens, best in their own context / transaction.

@oysterpack
Copy link

I would propose consolidating all of the open FT related NEPs into a single NEP the FT discussion - and close out the rest.

Let's create a new github repo that defines the contract interface. The goal of the new git repo would be that anybody that wants to implement the FT token standard interface can simply pull it into their project. Once we come to agreement on the contract interface then we can even officially publish the crate to https://crates.io

@evgenykuzyakov
Copy link
Contributor Author

evgenykuzyakov commented Jan 6, 2021

@evgenykuzyakov are you planning to add transfer_call to NEP-122? If not then let's continue the discussion in #136 .

No I don't think we should continue work on this NEP. At the same time #136 contains token metadata that I don't agree with. I would make sense to either remove some items from the metadata or remove metadata completely from the standard and move it to a different NEP.

Also account registration should be a separate NEP. @robert-zaremba if you agree, I suggest split NEP#136 into 3 NEPs to separately discuss the following:

  • transfers and view calls for balance and total supply - this is a core functionality of a token contract.
  • token metadata - metadata standard might change to expose additional information, the schema might evolve, but it doesn't mean the transfer protocol has to change. So it has to be a separate standard.
  • account registration to cover storage fees - this pattern will be reused for other contracts, so it better be standardized separately, so it can be reused.

I'll start working on 3 new NEPs to cover each part.

@robert-zaremba
Copy link
Contributor

@evgenykuzyakov - yes, this makes sens. The final FT token will be a merge of the 3 other interfaces.

@oysterpack
Copy link

perfect - we need to also close #136, #110, #102, #121

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

8 participants