Master fix member channel nft proceeds effectively credited #3763
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
case : NFT is member-channel owned and reward account is not set
There was a problem hiding this comment.
Overall looks good, but lets have @Lezek123 also review.
In various places we do channel_by_id reads inside various methods, and if these are called at the wrong time, where the channel is not guaranteed to exist, then this will create storage side-effect in such methods, so this is a bit risky in terms of future maintainers. It would be safer to just pass in channel objects directly to such methods, and then to read such objects in extrinsic scope using channel_by_id(video.channel_id) after it has been established that the video exists. This is less likely to break in the future.
| .reward_account | ||
| .ok_or_else(|| Error::<T>::RewardAccountIsNotSet.into()), | ||
| NftOwner::ChannelOwner => { | ||
| let channel = Self::ensure_channel_exists(&channel_id)?; |
There was a problem hiding this comment.
- This method needs a better name, lets call it
ensure_nft_owner_has_beneficiary_account. - It should not fail statically on reading channel from storage, please see general comment in primary review comment. Hence make
channela method parameter which is read usingvideo.channel_idinfallibly in extrinsic scope after video has been confirmed to exist.
There was a problem hiding this comment.
addressed in 8d223f3
| .ok_or_else(|| Error::<T>::RewardAccountIsNotSet.into()), | ||
| NftOwner::ChannelOwner => { | ||
| let channel = Self::ensure_channel_exists(&channel_id)?; | ||
| Self::ensure_reward_account(&channel) |
There was a problem hiding this comment.
Can we rename ensure_reward_account to ensure_channel_has_beneficiary_account.
There was a problem hiding this comment.
addressed in 8d223f3
|
Please apply cargo fmt: |
There was a problem hiding this comment.
The change looks ok, but I think in general the nft tests need some refactorization (probably not as part of this PR though).
There are quite a lot of combinations that need to be tested in order to make sure everything works as expected:
Ways to buy/acquire nft:
- through open auction (winner picked)
- through open auction (buy now price hit)
- through english auction (settle)
- through english auction (buy now price hit)
- through
accept_incoming_offer - through
buy_nft
For each of those we can have any combination of:
NFT owner beeing:
- a member
- a member channel w/ reward account set (original issuer)
- a member channel w/ reward account not set (original issuer)
- a curator channel w/ reward account set (original issuer)
- a curator channel w/ reward account not set (original issuer)
Video owner (royalty reciever) beeing:
- a member channel w/ reward account set
- a member channel w/ reward account not set
- a curator channel w/ reward account set
- a curator channel w/ reward account not set
If we wanted to test all possible combinations, that's 48 separate test cases (assuming we always use Some(_) royalty to test that part too).
Manually writing each test case would be quite cumbersome, but I think it would be possible to make some shared setup for initializing and testing all possible channel / nft owners and just have one test like buy_nft_all_combinations_ok for each of the 6 ways the nft can be acquired.
Some mock code example to ilustrate what I have in mind:
let channels = [
{
sender: DEFAULT_MEMBER_ACCOUNT_ID,
actor: ContentActor::Member(DEFAULT_MEMBER_ID))
reward_account: None,
expected_royalty_dst: DEFAULT_MEMBER_ACCOUNT_ID,
},
{
sender: DEFAULT_MEMBER_ACCOUNT_ID,
actor: ContentActor::Member(DEFAULT_MEMBER_ID))
reward_account: Some(DEFAULT_MEMBER_CHANNEL_REWARD_ACCOUNT_ID),
expected_royalty_dst: DEFAULT_MEMBER_CHANNEL_REWARD_ACCOUNT_ID,
},
{
sender: DEFAULT_CURATOR_ACCOUNT_ID,
actor: ContentActor::Curator(group_id, DEFAULT_CURATOR_ID))
reward_account: None,
expected_royalty_dst: None,
},
{
sender: DEFAULT_CURATOR_ACCOUNT_ID,
actor: ContentActor::Curator(group_id, DEFAULT_CURATOR_ID))
reward_account: Some(DEFAULT_CURATOR_CHANNEL_REWARD_ACCOUNT_ID),
expected_royalty_dst: DEFAULT_CURATOR_CHANNEL_REWARD_ACCOUNT_ID,
},
]
Now for each of channels you want to setup the nft owner to be either the original channel, or OTHER_MEMBER.
If the owner is the channel then expected_payment_dst = expected_royalty_dst, otherwise expected_payment_dst = OTHER_MEMBER_ACCOUNT_ID.
You could reuse a setup like this (creating channel, issuing nft w/ a specific owner) and final assertions for all 6 scenarios through which the nft can be acquired, making the number of test smaller and the tests themselves quite clear and complete.
| CreateChannelFixture::default() | ||
| .with_sender(DEFAULT_CURATOR_ACCOUNT_ID) | ||
| .with_actor(curator_actor) | ||
| .call(); |
There was a problem hiding this comment.
The method name should be accept_incoming_offer_with_no_reward_account_burns_token_with_curator_owner_channel
fixes #3762
No code has been lost, it is just that changes in #3650 have never been implemented. Royalty payment was working correctly however
Changes
complete_auctioninternal logic hence they can be treated as equal in this respectensure_owner_account_idhas been renamed toensure_nft_owner_account_idand internally usesensure_reward_account, effectively implementing the business logic desired: Revising destination of proceeds in NFT transactions #3650build_royalty_paymentwhich worked correctly already, I just refactored it by reusing theensure_reward_accountmethod