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

feat(NFT): Add option types and add missing memo parameter #276

Merged
merged 16 commits into from
Nov 18, 2022

Conversation

willemneal
Copy link
Contributor

@willemneal willemneal commented Nov 17, 2021

This adds the missing memo argument needed by nft_transfer and changes types inspired by Paras' usage. Notably using a type alias for Payout of a hashmap. Also adds more options so that the API is less strict.

https://github.com/ParasHQ/paras-marketplace-contract/blob/ac35d84a1af7ea369c3916db32af0dd1e5a43024/paras-marketplace-contract/src/external.rs#L7-L14

 fn nft_transfer_payout(
        &mut self,
        receiver_id: AccountId,
        token_id: TokenId,
        approval_id: Option<u64>,
        memo: Option<String>,  // This is ignored by Paras
        balance: U128,
        max_len_payout: Option<u32>,
    );
  • Need to specify that a nft_transfer_payout that returns a bad "payout" should still payout to old token owner

@willemneal
Copy link
Contributor Author

@mikedotexe @mattlockyer Thoughts?

@emarai
Copy link

emarai commented Dec 2, 2021

I'm unsure why the balance would be optional, so if they could explain their choice that would be great.

Hi @willemneal , regarding why the balance is optional, we picked it up from near-examples/nft-market. But it seems it has been changed to be no longer optional (https://github.com/near-examples/nft-market/commit/ba2b5ce07043061059a5482e815433101d4c455f#diff-bd87736f4e1006e27d541b8adc5bc34e439b4c1ce8c234cefc752408d01eb369L130)

@microchipgnu
Copy link

I agree with removing the wrapper on Payout. Don't see its utility.

specs/Standards/NonFungibleToken/Payout.md Outdated Show resolved Hide resolved
specs/Standards/NonFungibleToken/Payout.md Outdated Show resolved Hide resolved
@willemneal willemneal requested a review from BenKurrek December 14, 2021 16:16
@willemneal willemneal changed the title feat(NFT): Change type of Payout to hashmap and add option types feat(NFT): Add option types and add missing memo parameter Dec 23, 2021
@willemneal
Copy link
Contributor Author

I reverted the payout type from this PR. It can be discussed in the future. However, this PR does fix several issues with the current standard, which shouldn't be breaking changes to those that have implemented it. For example, an Option type is encoded the same way as a normal one just isn't a required parameter. Furthermore the memo parameter was missing which is required for nft_transfer.

@mikedotexe @BenKurrek

@willemneal
Copy link
Contributor Author

@mikedotexe Who should I contact on the Dev Platform team? Is there a Dev Platform team?

@willemneal willemneal requested a review from mikedotexe January 28, 2022 18:59
@willemneal
Copy link
Contributor Author

@mikedotexe

2 similar comments
@willemneal
Copy link
Contributor Author

@mikedotexe

@willemneal
Copy link
Contributor Author

@mikedotexe

@willemneal
Copy link
Contributor Author

@frol frol requested review from a team and abacabadabacaba October 5, 2022 13:33
@frol
Copy link
Collaborator

frol commented Oct 5, 2022

@near/wg-contract-standards I feel this NEP is well-received, so we can cast a vote here.

Copy link
Member

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

LGTM.

@frol frol added S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. and removed S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. labels Oct 7, 2022
@frol
Copy link
Collaborator

frol commented Oct 7, 2022

We are reaching a consensus, but there is still a chance to influence the decision. I have transitioned this proposal to the final-comment period, and we will make the final vote on the next Contract Standards Working Group call, which will be the last chance to speak before we merge this PR.

@ori-near ori-near added the A-NEP A NEAR Enhancement Proposal (NEP). label Oct 13, 2022
@ori-near
Copy link
Contributor

@near/wg-contract-standards – As @frol mentioned above, we would like to schedule a working group meeting to review this NEP. Before we proceed, we would like to get closer to a consensus on this NEP.

@abacabadabacaba Could you please fully read this NEP and comment in the thread if you are leaning with approving or rejecting it? Please make sure to include your rationale and any feedback that you have for the author. Thank you.

@ori-near ori-near removed the help wanted Extra attention is needed label Nov 4, 2022
@abacabadabacaba
Copy link
Collaborator

I lean with approving this NEP. Adding a memo parameter seems useful given that the underlying transfer function takes it, and making max_len_payout optional also makes sense.

@frol
Copy link
Collaborator

frol commented Nov 11, 2022

Just to resolve ambiguity, I want to re-emphasize that I lean towards approving this NEP extension as it brings useful features in a backward-compatible way, so there is no risk of breaking existing ecosystem.

@ori-near ori-near added A-NEP-Extension A new functionality proposal to existing NEP. Once original author merges changes, we close this. and removed A-NEP A NEAR Enhancement Proposal (NEP). labels Nov 11, 2022
@ori-near
Copy link
Contributor

As the moderator, I would like to thank the author @willemneal for submitting this NEP extension, and for the @near/wg-contract-standards working group members for your review. Based on the voting indications above, it seems like this proposal is close to a decision. We are therefore scheduling the second Contract Standards Working group meeting next week, where this NEP can enter the final voting stage. We encourage anyone who is interested to join the meeting.

Meeting Info
📅 Date: Friday, Nov 18, 4pm UTC
🗒️ Topic: NEP-276 Voting
✏️ Register

specs/Standards/NonFungibleToken/Payout.md Outdated Show resolved Hide resolved
specs/Standards/NonFungibleToken/Payout.md Outdated Show resolved Hide resolved
@willemneal willemneal requested a review from a team as a code owner November 17, 2022 16:01
@willemneal willemneal force-pushed the feat/nft_payouts_data_structure branch from 2494928 to 3859e71 Compare November 17, 2022 16:19
@willemneal willemneal force-pushed the feat/nft_payouts_data_structure branch from 3859e71 to 1882887 Compare November 17, 2022 16:20
@frol frol self-requested a review November 18, 2022 12:29
@ori-near
Copy link
Contributor

ori-near commented Nov 18, 2022

Thank you to everyone who attended the second Contract Standards Working Group meeting today! The working group members reviewed the NEP and reached the following consensus:

Status: Approved

Meeting Recording

@willemneal Thank you for authoring this NEP!

Next Steps:

Implementation Status:
NEAR SDK Rust: PR is in the final stage waiting the author to address the final comments (potentially needs to be forked to run the last mile) near/near-sdk-rs#778
NEAR SDK JavaScript: Awaiting community contribution

Community contributions to the SDKs are welcome!

@ori-near ori-near merged commit 3a2fd98 into near:master Nov 18, 2022
@frol frol mentioned this pull request Nov 18, 2022
@frol frol added S-approved A NEP that was approved by a working group. and removed S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. labels Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP-Extension A new functionality proposal to existing NEP. Once original author merges changes, we close this. S-approved A NEP that was approved by a working group. WG-contract-standards Contract Standards Work Group should be accountable
Projects
Status: APPROVED FIXES
Development

Successfully merging this pull request may close these issues.

10 participants