Reference implementation for proposed Multi-token spec (NEP-245) #950

uncle-T0ny · 2022-10-27T17:35:30Z

This PR is based on #776 implementing Multi-token Standard (near/NEPs#245, near/NEPs#227, near/NEPs#246)
with the updated codebase and the improvements.

The improvements list (by commits):

MT token: update to the latest codebase

apply all the bugfixes in other standards that affect multi-token
update the MT by using a new way for XCC and use the "abi" feature

MT token: fix mt_is_approved, make method viewable

in mt_is_approved view method can't use predecessor_account_id, added owner_id as a param

MT token: fix the approval invalidation, reduce the approved amount

fixed an approve invalidation bug, also decrease the approved amount after the transfer

MT token: added storage management, fix method types, refactor e2e tests

implement Storage Management for MT token
in some methods changed u128 to the wrapped version U128 for compatibility with the JS
improved e2e tests and added more cases

frol

This is a massive PR, so I will review it in chunks. Please, bear with me

examples/multi-token/test-approval-receiver/src/lib.rs

examples/multi-token/test-contract-defi/src/lib.rs

frol · 2022-11-16T13:28:43Z

examples/multi-token/test-contract-defi/src/lib.rs

+        token_ids: Vec<TokenId>,
+        amounts: Vec<U128>,


What are the Pros of having these two fields instead of a single field with a Vec<(TokenId, U128)>?

From my understanding the interface borrowed from eip-1155 method: onERC1155BatchReceived
I didn't find any proposal to change from the discussions and the meetings.

frol · 2022-11-16T13:51:20Z

examples/multi-token/test-contract-defi/src/lib.rs

+        token_ids: Vec<TokenId>,
+        amounts: Vec<U128>,
+        msg: String,
+    ) -> PromiseOrValue<Vec<U128>> {


I would argue that it could be error-prone to only return balances without the token ids, so I would like to propose returning Vec<(TokenId, U128)>.

Also, the standard should define what to do if the returned list does not contain some of the tokens, i.e. if the request was [["TKN1", "10"], ["TKN2", "1"]] while the returned value is [["TKN2", "0"]] or even a completely empty list [], and while we are speaking of corner cases, what to do if the returned value mentions the token that was not in the input ([["TKN_NOT_REQUESTED", "3"]])

From the NEP-245

Take some action after receiving a multi token Requirements: * Contract MUST restrict calls to this function to a set of whitelisted contracts * Contract MUST panic if `token_ids` length does not equals `amounts` length * Contract MUST panic if `previous_owner_ids` length does not equals `token_ids` length

But, you provided a nice case, I think It would be nice to add more test cases.

from other side returning Vec<(TokenId, U128)> not safe because a predecessor can change the token

from other side returning Vec<(TokenId, U128)> not safe because a predecessor can change the token

What do you mean?

Well, thinking about it a bit more, I feel that the list of amounts could be cheaper/easier to implement, so maybe it is not worth complicating things

I kept parameters as-is based on the comments above. Agree with @frol that prioritizing simplicity makes sense here

near-contract-standards/src/multi_token/approval/approval_impl.rs

… issues

frol

Thanks for addressing the previous review comments!

frol · 2022-11-28T15:30:01Z

examples/multi-token/mt/src/lib.rs

+
+    #[init]
+    pub fn new(owner_id: AccountId, metadata: MtContractMetadata) -> Self {
+        require!(!env::state_exists(), "Already initialized");


Since near-sdk-rs v3.0.1:

BREAKING #[init] now checks that the state is not initialized. This is expected behavior. To ignore state check you can call #[init(ignore_state)]

Thus, you don't need this check:

Suggested change

require!(!env::state_exists(), "Already initialized");

removed this

frol · 2022-11-28T15:55:50Z

examples/multi-token/test-contract-defi/src/lib.rs

+        token_ids: Vec<TokenId>,
+        amounts: Vec<U128>,
+        msg: String,
+    ) -> PromiseOrValue<Vec<U128>> {


from other side returning Vec<(TokenId, U128)> not safe because a predecessor can change the token

What do you mean?

Well, thinking about it a bit more, I feel that the list of amounts could be cheaper/easier to implement, so maybe it is not worth complicating things

uncle-T0ny · 2023-01-24T01:26:45Z

Update:
Added extension to have a possibility to use Royalties and Payouts interface, like we have in NFT contract
MT: added token holders extension

Kirill-K-1 · 2023-03-23T09:35:23Z

near-contract-standards/src/multi_token/approval/approval_impl.rs

+            let grantee_to_approval = match by_owner.get(&owner_id) {
+                Some(grantee_to_approval) => grantee_to_approval,
+                None => return false,
+            };
+
+            let approval = match grantee_to_approval.get(&approved_account_id) {
+                Some(approval) => approval,
+                None => return false,
+            };
+
+            if !approval.amount.eq(&amount.into()) {
+                return false;
+            }


Suggested change

let grantee_to_approval = match by_owner.get(&owner_id) {

Some(grantee_to_approval) => grantee_to_approval,

None => return false,

};

let approval = match grantee_to_approval.get(&approved_account_id) {

Some(approval) => approval,

None => return false,

};

if !approval.amount.eq(&amount.into()) {

return false;

}

let approval = match by_owner

.get(&owner_id)

.and_then(|grantee_to_approval| grantee_to_approval.get(&approved_account_id))

{

Some(approval) if approval.amount.eq(&amount.into()) => approval,

_ => return false,

};

Made this change

Kirill-K-1 · 2023-03-24T16:21:27Z

near-contract-standards/src/multi_token/token.rs

+
+// Represents a temporary record of an Approval
+// that was removed from the ApprovalContainer but may be restored in case of rollback in XCC.
+// Values are (owner_id, approval_id, amount)


I think this tuple description should be changed, what do you think?

Updated the description

Kirill-K-1 · 2023-03-24T16:28:21Z

near-contract-standards/src/multi_token/core/core_impl.rs

+    pub extra_storage_in_bytes_per_emission: StorageUsage,
+
+    /// Owner of each token
+    pub owner_by_id: TreeMap<TokenId, AccountId>,


What is the reason of using TreeMap here?

@Kirill-K-1 What do you propose is best data structure? Would you be fine with UnorderedMap so it can be iterated through?

Kirill-K-1 · 2023-03-24T16:36:18Z

near-contract-standards/src/multi_token/enumeration/enumeration_impl.rs

+            self.owner_by_id.len() as u128 >= start_index,
+            "Out of bounds, please use a smaller from_index."
+        );
+        let limit = limit.map(|v| v as usize).unwrap_or(usize::MAX);


How about to use

let limit = limit.unwrap_or(u64::MAX);

nearbuild · 2023-04-19T16:30:24Z

Can we schedule a call in the Protocol Community call this month reviewing everything outlined in the Multitoken standard? https://near.social/#/devgovgigs.near/widget/gigs-board.pages.Post?id=376

uint · 2023-07-11T13:19:36Z

@uncle-T0ny I'm looking through old PRs. Is this one something you'd like to keep working on and finalize? I understand if not - it looks like it moved slowly before.

Address comments

uncle-T0ny added 5 commits October 26, 2022 15:11

use jriemann MT ref. implementation

6d79045

MT token: update to the latest codebase

c22daf6

MT token: fix mt_is_approved, make method viewable

e7cabb4

MT token: fix the approval invalidation, reduce approve amount

438ec7a

MT token: added storage management, fix method types, refactor e2e tests

8ff8d46

uncle-T0ny mentioned this pull request Oct 28, 2022

Multi Token Contract Standard near/NEPs#246

Open

frol reviewed Nov 17, 2022

View reviewed changes

uncle-T0ny added 2 commits November 21, 2022 13:36

MT token: refund approval after failed transfer, fixed clippy and fmt…

083ea27

… issues

MT token: code review fixes

0eeb424

frol reviewed Nov 28, 2022

View reviewed changes

frol mentioned this pull request Dec 14, 2022

feat: Multi Token Standard Contract [NEP] near/NEPs#245

Merged

uncle-T0ny added 2 commits December 14, 2022 13:38

MT token: added missed param to mt_on_approve

903aa8d

MT: added token holders extension

189de1b

Kirill-K-1 reviewed Mar 23, 2023

View reviewed changes

Kirill-K-1 reviewed Mar 24, 2023

View reviewed changes

uint mentioned this pull request Jul 11, 2023

Reference implementation for proposed Multi-token spec (NEP-245) #776

Closed

kenobijon added 2 commits September 28, 2023 18:49

address outstanding comments

bb2f219

modified comment for ClearedApproval

e21c180

kenobijon mentioned this pull request Sep 29, 2023

Address comments TENAMINT/near-sdk-rs-plus-mt#1

Merged

kenobijon added 2 commits September 28, 2023 19:26

Merge pull request #1 from TENAMINT/address-comments

d270550

Address comments

Merge branch 'near:master' into multi-token-reference-impl

1d6b224

ruseinov mentioned this pull request Nov 15, 2023

[Feature] Multi Token standard implementation NEP-245. #1111

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference implementation for proposed Multi-token spec (NEP-245) #950

Reference implementation for proposed Multi-token spec (NEP-245) #950

uncle-T0ny commented Oct 27, 2022 •

edited by frol

Loading

frol left a comment

frol Nov 16, 2022

uncle-T0ny Nov 21, 2022

frol Nov 16, 2022

uncle-T0ny Nov 21, 2022

uncle-T0ny Nov 22, 2022

frol Nov 28, 2022

kenobijon Sep 29, 2023

frol left a comment

frol Nov 28, 2022

kenobijon Sep 29, 2023

frol Nov 28, 2022

uncle-T0ny commented Jan 24, 2023

Kirill-K-1 Mar 23, 2023

kenobijon Sep 29, 2023

Kirill-K-1 Mar 24, 2023

kenobijon Sep 29, 2023

Kirill-K-1 Mar 24, 2023

kenobijon Sep 27, 2023

Kirill-K-1 Mar 24, 2023

kenobijon Sep 29, 2023

nearbuild commented Apr 19, 2023

uint commented Jul 11, 2023

Reference implementation for proposed Multi-token spec (NEP-245) #950

Are you sure you want to change the base?

Reference implementation for proposed Multi-token spec (NEP-245) #950

Conversation

uncle-T0ny commented Oct 27, 2022 • edited by frol Loading

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uncle-T0ny commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nearbuild commented Apr 19, 2023

uint commented Jul 11, 2023

uncle-T0ny commented Oct 27, 2022 •

edited by frol

Loading