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/payout #778

Closed
wants to merge 6 commits into from
Closed

Feat/payout #778

wants to merge 6 commits into from

Conversation

ymc182
Copy link

@ymc182 ymc182 commented Apr 4, 2022

Takes #673 closer to the final stage

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Quick scan. @jlogelin can you review that this matches the spec?

I don't have context on this and unclear how it's assumed the royalty percentage would be set/updated. Also didn't check closely if this matches the spec

@@ -2,7 +2,7 @@
members = [
"near-sdk",
"near-sdk-macros",
"near-sdk-sim",
"near-sdk-sim",
Copy link
Contributor

Choose a reason for hiding this comment

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

can you revert this spacing?

Copy link
Author

Choose a reason for hiding this comment

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

No Problem

@@ -426,7 +431,10 @@ impl NonFungibleTokenCore for NonFungibleToken {
msg: String,
) -> PromiseOrValue<bool> {
assert_one_yocto();
require!(env::prepaid_gas() > GAS_FOR_NFT_TRANSFER_CALL, "More gas is required");
require!(
env::prepaid_gas() > GAS_FOR_NFT_TRANSFER_CALL + GAS_FOR_RESOLVE_TRANSFER,
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this change please

@@ -12,6 +12,9 @@ mod macros;
/// Metadata traits and implementation according to the [NFT enumeration standard](https://nomicon.io/Standards/NonFungibleToken/Metadata.html).
/// This covers both the contract metadata and the individual token metadata.
pub mod metadata;

pub mod payout;
Copy link
Contributor

Choose a reason for hiding this comment

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

Module isn't documented here or in .../payout/mod.rs,

can you describe this and link to the spec there using //! rustdoc comments at the top of the file, please?

Comment on lines +9 to +13
/* pub fn new(accounts: HashMap<AccountId, u8>, percent: u8) -> Self {
let this = Self { accounts, percent };
this.validate();
this
} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* pub fn new(accounts: HashMap<AccountId, u8>, percent: u8) -> Self {
let this = Self { accounts, percent };
this.validate();
this
} */

Comment on lines +7 to +8
use std::collections::HashMap;
impl Royalties {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use std::collections::HashMap;
impl Royalties {
use std::collections::HashMap;
impl Royalties {

}

fn apply_percent(percent: u8, int: u128) -> u128 {
int * percent as u128 / 100u128
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this can overflow, are there any guarantees around this? Probably want to do a checked multiplication with an expect to make sure someone doesn't need overflow-checks enabled to catch this

pub struct Royalties {
key_prefix: Vec<u8>,
pub accounts: HashMap<AccountId, u8>,
pub percent: u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

#[serde(crate = "near_sdk::serde")]
pub struct Royalties {
key_prefix: Vec<u8>,
pub accounts: HashMap<AccountId, u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be basis points.

Copy link
Author

Choose a reason for hiding this comment

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

No Problem

@eternal-ai-org
Copy link

How long this feature should be merged into?

@willemneal
Copy link
Contributor

@thaibao-incognito, there is no timeline. If you want to use this today, here is a complete example: https://github.com/TENK-DAO/tenk/blob/main/contracts/tenk/src/payout.rs

@frol
Copy link
Collaborator

frol commented Nov 18, 2022

FYI, Payout NFT standard was aligned with the implementation: near/NEPs#276

This PR is almost there, and it seems only the last mile is left, so I hope NEAR community can make this final push here

@ruseinov
Copy link
Collaborator

ruseinov commented Oct 2, 2023

@frol I think this can now be closed in favour of #1077

@frol frol closed this Oct 22, 2023
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.

6 participants