feat(near-contract-standards): NEP-199 - Non-Fungible Token Royalties and Payouts #1077
Conversation
|
@frol I'm not sure if the |
frol
changed the title
|
@ruseinov May I ask you to submit a new PR to fix the CI failure? (I believe it is not related to this change) |
definitely! |
This seems to only be reproducible on this branch. |
Cheers! Fixed! |
| // TODO: Figure out where the percent has to be coming from. | ||
| Royalties::new(prefix, 0) |
There was a problem hiding this comment.
So, currently the royalties.payout.percent is a public field, and it seems that the expectation is that developers using this extension will adjust it. I find this contract-level percentage quite inflexible - 0% shared between all payouts is useless, 1-99% could be limiting or confusing, and 100% seems to be the only reasonable option, but then payout to the token owner needs to be explicitly listed (storage wasted).
In either cases, please, provide usage-level documentation on how to use this stuff.
There was a problem hiding this comment.
I was thinking that this could be done in a different way. Why don't we sum all the payouts and send the rest to the owner:
pub fn create_payout(&self, balance: Balance, owner_id: &AccountId) -> Payout {
let mut payout = Payout {
payout: self
.accounts
.iter()
.map(|(account, percent)| (account.clone(), apply_percent(percent, balance).into()))
.collect(),
};
let rest = balance - payout.payout.values().fold(0, |acc, &sum| acc + sum);
let owner_payout: u128 = payout.payout.get(owner_id).map_or(0, |x| x.0) + rest;
payout.payout.insert(owner_id.clone(), owner_payout.into());
payout
}
|
@frol @agostbiro This needs to be re-approved and merged if all good. |
Right, after updating to latest it needs some small fixes, I'll take care of those. |
|
Done. |
|
@frol seems to be fixed, some flaky example tests due to |
| let mut payout = Payout { | ||
| payout: self | ||
| .accounts | ||
| .iter() | ||
| .map(|(account, percent_per_token)| { | ||
| ( | ||
| account.clone(), | ||
| U128::from(apply_percent( | ||
| *percent_per_token.get(&token_id).unwrap_or(&0), | ||
| balance, | ||
| )), | ||
| ) | ||
| }) | ||
| .filter(|(_, payout)| payout.0 > 0) | ||
| .collect(), | ||
| }; | ||
| let rest = balance - payout.payout.values().fold(0, |acc, &sum| acc + sum.0); | ||
| let owner_payout: u128 = payout.payout.get(owner_id).map_or(0, |x| x.0) + rest; | ||
| payout.payout.insert(owner_id.clone(), owner_payout.into()); |
There was a problem hiding this comment.
Something crazy is going on here. Royalties should be just an UnorderedMap<TokenId, HashMap<AccountId, Percentage>>, and then create_payout will be just:
royalties.get(token_id)
.items()
.map(|(account_id, percentage)| (account_id, balance * percentage / 100))
.collect()There was a problem hiding this comment.
@ruseinov Also, as a rule of thumb, if you write a code that iterates over near_sdk collections without bounds and potentially may exceed more than 100 lookups, you need to redesign the approach as that won't work with the growing number records. In this case, as I mentioned above, it needs a completely different layout
There was a problem hiding this comment.
My mistake, I made a fast fix of a problem that completely redefined the solution.
There was a problem hiding this comment.
Actually, the truth is that the original map is bound by validate method, it only allows for 10 accounts. So technically it's not an issue.
The data structure and it's access are a little bit convoluted, but if we were to redesign this collection and the approach - we'd have to discuss how we are going to validate bounds going forward.
| /// impl NonFungibleTokenPayout for Contract { | ||
| /// #[allow(unused_variables)] | ||
| /// fn nft_payout( | ||
| /// &self, | ||
| /// token_id: String, | ||
| /// balance: U128, | ||
| /// max_len_payout: Option<u32>, | ||
| /// ) -> Payout { | ||
| /// self.tokens.nft_payout(token_id, balance, max_len_payout) | ||
| /// } | ||
| /// #[payable] | ||
| /// fn nft_transfer_payout( |
There was a problem hiding this comment.
I realized that most of the examples lack at least demo methods on how to mint tokens and, in this case, how to assign royalties to the tokens. This would have highlighted the problem that to assign royalties one would need to access internal fields deep down and manipulate them manually:
self.tokens.royalties.unwrap().royalties.entry(token_id).insert(royalties_to_account_id, 5)Without this example, this implementation most probably won't be used and we may even argue that we may not need to merge it in the first place.
There was a problem hiding this comment.
Yes, I have actually been thinking about that too when I first glanced at the original PR, but then it got forgotten as I moved forward.
We do indeed have to expose an interface to assign royalties.
There was a problem hiding this comment.
Just going to leave this here: what we need to do is introduce a create_royalties method or similar that will only be accessible to nft owner.
A continuation of #778 - NEP-199 standard implementation
This needs tests for the examples.
It also has one failing test that seemingly has nothing to do with the changes.