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

Update fungible token example to use basic NEP 141 #289

Closed
wants to merge 2 commits into from

Conversation

mikedotexe
Copy link
Contributor

@mikedotexe mikedotexe commented Feb 11, 2021

This overwrites the old fungible token example that used NEP-21 with NEP-141.
near/NEPs#141

There's a great summary here:
https://learn.figment.io/network-documentation/near/tutorials/1-project_overview/2-fungible-token

That summary above was gleaned from discussions here:

This is similar to this draft PR, except it's removed all concepts of a "wrapped NEAR" project. Actually very trivial changes to get to this point.

I'd like to post this as a draft because I'd like to flesh out with simulation tests and write the README. However, I believe that this will be enough to unblock the EVM team members and Bridge members looking for an implementation of this new fungible token standard.

@alexauroradev
Copy link

CC: @mfornet, this will be useful for the FT connector migration

@chadoh
Copy link
Contributor

chadoh commented Feb 11, 2021

Should reference NEP146 in place of 141, right?

@mfornet
Copy link
Member

mfornet commented Feb 11, 2021

I think it would be great to have reference vanilla implementation of this contract in https://github.com/near/core-contracts/ rather than in examples here, and of course maintain a sound version of it, so developers can reuse that code confidently.

@oysterpack
Copy link

Account Storage Standard API is ready for final review.

Here's where we stand with for Fungible Token Metadata. There's at least one open question to resolve before we can put it out there for final review. Please review and provide feedback.

NOTE: we have a standards review meeting scheduled for 11 AM ET / 4 PM GMT (organized by @mikedotexe). On the meeting agenda, we have the Account Storage Standard API and Fungible Token Metadata. Please review and cast your votes on the discussions. Feedback is appreciated.


#[derive(Serialize, BorshDeserialize, BorshSerialize, Clone)]
#[serde(crate = "near_sdk::serde")]
pub struct FungibleTokenMetadata {

Choose a reason for hiding this comment

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

FungibleTokenMetadata struct is not yet finalized and still WIP


/// Price per 1 byte of storage from mainnet config after `0.18` release and protocol version `42`.
/// It's 10 times lower than the genesis price.
pub const STORAGE_PRICE_PER_BYTE: Balance = 10_000_000_000_000_000_000;

Choose a reason for hiding this comment

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

storage price is hard coded - but storage price may change over time

pub trait StorageManager {
fn storage_deposit(&mut self, account_id: Option<ValidAccountId>) -> AccountStorageBalance;

fn storage_withdraw(&mut self, amount: U128) -> AccountStorageBalance;

Choose a reason for hiding this comment

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

per the proposed standard, amount should be optional:

fn storage_withdraw(&mut self, amount: Option<U128>) -> AccountStorageBalance;

}

#[payable]
fn storage_withdraw(&mut self, amount: U128) -> AccountStorageBalance {

Choose a reason for hiding this comment

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

Something to call out here is that storage_withdraw() implementation here is specific to this FT use case. It is being used to "unregister" or "close" the account for this FT use case. Note that I say for "this" FT use case because different business rules may apply based on the specific FT use case, i.e., not all FT contracts may want this behavior.

In general, storage_withdraw() should only perform what it states, i.e., withdraw from the account's storage available balance. It should not be used to delete the account state from the contract. Closing the account should be performed explicitly via a separate function that makes it clear what the intent is.

assert_eq!(
amount,
self.storage_minimum_balance().0,
"Requires attached deposit of the exact storage minimum balance"

Choose a reason for hiding this comment

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

Something to call out is that this storage_deposit() implementation is specific to this FT use case, i.e., requiring exact amount that matches storage_minimum_balance(). The business logic depends on the contract's context and use case.

@ilblackdragon
Copy link
Member

ilblackdragon commented Feb 21, 2021

Hey @mikedotexe didn't see this but I've been working on making a library in #275 instead of keeping all the code in the examples and people copy pasting it to their implementations.

See NEP-141 in this library: #293
We should merge your work from this PR into the library, as library allows really quickly build up custom token implementations without copy paste.

I also think that near/core-contracts#128 should be then modified to use this library as one of the simplest examples.

@mikedotexe
Copy link
Contributor Author

Hey @mikedotexe didn't see this but I've been working on making a library in #275 instead of keeping all the code in the examples and people copy pasting it to their implementations.

See NEP-141 in this library: #293
We should merge your work from this PR into the library, as library allows really quickly build up custom token implementations without copy paste.

I also think that near/core-contracts#128 should be then modified to use this library as one of the simplest examples.

Roger that! We're all aligned on this and Eugene will be modifying that #293 code as needed. This is our top priority.

@evgenykuzyakov
Copy link
Contributor

Closing in favor of #275

@austinabell austinabell deleted the hotfix/update-ft branch April 12, 2022 15:33
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

Successfully merging this pull request may close these issues.

7 participants