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

Fix nep245 #511

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Fix nep245 #511

wants to merge 9 commits into from

Conversation

marco-sundsk
Copy link
Contributor

Now, we have a revision of this standard. This time, we focus on the combination of FT and NFT in those core functions cause we noticed some misunderstandings on FT and NFT workflow on near network in those interfaces. Beside that, we also make a supplement to the nep-0245.md to include all the extensions while leaving the one in specs untouched as a comparision base point, so that you could have a clear view about what have been changed in this revision.

The plan is to have an agreement on nep-0245.md first, then the corresponding fix would be applied to those in specs. And the standard code implementation would be the last.

The detailed explaination of this revision could be found in the discussion.

@@ -326,14 +309,11 @@ The following behavior is required, but contract authors may name this function
// `approvals` is an array of expected `approval_list` per `token_ids`.
// If a `token_id` does not have a corresponding `approvals_list` then the entry in the
// array must be marked null.
// `approvals_list` is an array of triplets of [`owner_id`,`approval_id`,`amount`].
// `owner_id` is the valid Near account that owns the tokens.
// `approvals_list` is an array of triplets of [`approved_account_id`,`approval_id`].

Choose a reason for hiding this comment

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

Suggested change
// `approvals_list` is an array of triplets of [`approved_account_id`,`approval_id`].
// `approvals_list` is an array of tuples of [`approved_account_id`,`approval_id`].

// For base info, either base_id or base would be included, depends on implementation.
// Suggest to return base to save client from extral query on base info.
type MTTokenMetadataAll = {
base_id: string | null, // id in type MTBaseTokenMetadata

Choose a reason for hiding this comment

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

This seems confusing, should this be instead restricted to one or the other (base_id vs including the metadata itself)?

type Token = {
token_id: string,
owner_id: string | null
owner_id: string | null, // not null if the token is a NFT
metadata: MTTokenMetadataAll | null // not null if Metadata extension is enabled

Choose a reason for hiding this comment

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

How/when does this field get assigned to a token?

Choose a reason for hiding this comment

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

Curious if we need to deal with Metadata at all at this stage, wouldn't it be nicer to go with iterative approach and get the barebone tokens right up to the contract implementation and then deal with extensions?

amount: string,
approval: [owner_id: string, approval_id: number]|null,
amount: string,
approval_id: number|null,

Choose a reason for hiding this comment

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

Same goes for approvals, do we really want this to be a part of the interface.
Why don't we just do the barebones MT standard without the extensions and then figure out how those will play together with what we have?

Comment on lines +386 to +387
- NEAR's [Fungible Token Metadata Standard](../FungibleToken/Metadata.md)
- NEAR's [Non-Fungible Token Metadata Standard](../NonFungibleToken/Metadata.md)

Choose a reason for hiding this comment

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

Those links seem broken.

@frol frol added WG-contract-standards Contract Standards Work Group should be accountable S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. A-NEP A NEAR Enhancement Proposal (NEP). S-review/needs-author-revision A NEP in the REVIEW stage that needs an author revision. S-draft/needs-implementation A NEP in the DRAFT stage that needs implementation. and removed S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. S-review/needs-author-revision A NEP in the REVIEW stage that needs an author revision. labels Nov 15, 2023
@frol frol marked this pull request as draft November 15, 2023 21:03
@frol
Copy link
Collaborator

frol commented Nov 15, 2023

Hi @marco-sundsk @ruseinov, thank you for pushing this NEP forward! I see that this is still an on-going effort, so as a NEP moderator, I marked this PR as draft and marked that it needs reference implementation (as per our discussion in Telegram, we don't need a full SDK-grade library implementation, we just need a standalone contract that implements all these methods and some client that demonstrates that the API surface is not lacking and ergonimic)

Please ping the @near/nep-moderators once you are ready for us to review it.

@victorchimakanu
Copy link

Hi @marco-sundsk @ruseinov, i'd like to understand the current status of this NEP.

@ruseinov
Copy link

ruseinov commented Jun 4, 2024

Hi @marco-sundsk @ruseinov, i'd like to understand the current status of this NEP.

The status is that we have a small MVP in another repo that's supposed to drive this effort. I'm currently busy with other things, but planning to eventually come back to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP A NEAR Enhancement Proposal (NEP). S-draft/needs-implementation A NEP in the DRAFT stage that needs implementation. WG-contract-standards Contract Standards Work Group should be accountable
Projects
Status: DRAFT
Development

Successfully merging this pull request may close these issues.

None yet

4 participants