Skip to content

🐛 Depositing PalletAttributeSet on incorrect nft#2740

Merged
ggwpez merged 9 commits intoparitytech:masterfrom
vikiival:fix/mint-pallet-attribute
Mar 9, 2024
Merged

🐛 Depositing PalletAttributeSet on incorrect nft#2740
ggwpez merged 9 commits intoparitytech:masterfrom
vikiival:fix/mint-pallet-attribute

Conversation

@vikiival
Copy link
Contributor

@vikiival vikiival commented Dec 18, 2023

Context

Implementing HolderOf(collection_id) we have observed a fancy glitch where pallet deposits event with incorrect values

Test case

Observe following extrinsic

To mint in collection 51 user needs to be HolderOf(50).
Therefore current user is owner of item 394 witness_data { owned_item: 394 }

All checking is done correctly, storage is updated correctly

photo_2023-12-18 16 07 11

However the event which is emitted does not make semantic sense as we updated storage for 50-394 not for 51-114

photo_2023-12-18 16 07 17

The fix

This PR fixes that depositing PalletAttributeSet emits correct values.

@vikiival vikiival requested a review from jsidorenko as a code owner December 18, 2023 15:13
@vikiival vikiival requested a review from a team December 18, 2023 15:13
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 18, 2023

User @vikiival, please sign the CLA here.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 18, 2023 15:13
@vikiival vikiival mentioned this pull request Dec 18, 2023
Closed
@jsidorenko jsidorenko added R0-no-crate-publish-required The change does not require any crates to be re-published. T2-pallets This PR/Issue is related to a particular pallet. labels Dec 19, 2023
jsidorenko
jsidorenko approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@jsidorenko jsidorenko left a comment

Choose a reason for hiding this comment

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

Agree, semantically it's better to emit the event with the params you've added.

@vikiival
Copy link
Contributor Author

vikiival commented Dec 19, 2023

Just want to ask is the CLA signed?

I signed it yesterday, with success. But seems bot has not updated its state.

I was trying to do it one more time but it failed

Screenshot 2023-12-19 at 12 45 30

ggwpez
ggwpez approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A test maybe? I think the CLA bot is happy.

@bkchr
Copy link
Member

bkchr commented Dec 19, 2023

@vikiival can you add a test?

liamaharon
liamaharon approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

LGTM once test is added

@vikiival
Copy link
Contributor Author

vikiival commented Dec 20, 2023

@vikiival can you add a test?

Will try ^-^ 💯

@bkchr
Copy link
Member

bkchr commented Dec 20, 2023

Ty @vikiival! Just search for assert_last_event to get some examples!

@vikiival
Copy link
Contributor Author

vikiival commented Feb 21, 2024

Hello,
2 months passed since I have opened this, unfortunatelly it is outside of my capacity to write a unit test from scratch.

However, this issue prevents from correctly indexing NFT events from the chain 🥺.
REF: kodadot/nft-gallery#9482

Therefore I would like to ask if someone more capable is able to write the test or if we can merge it as it is minor change

@vikiival vikiival mentioned this pull request Feb 21, 2024
Open
@jsidorenko
Copy link
Contributor

jsidorenko commented Feb 29, 2024

Hello, 2 months passed since I have opened this, unfortunatelly it is outside of my capacity to write a unit test from scratch.

However, this issue prevents from correctly indexing NFT events from the chain 🥺. REF: kodadot/nft-gallery#9482

Therefore I would like to ask if someone more capable is able to write the test or if we can merge it as it is minor change

Done, test added

@vikiival
Copy link
Contributor Author

vikiival commented Feb 29, 2024

@bkchr @liamaharon can we merge? 🥺🥹

@bkchr bkchr added this pull request to the merge queue Feb 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 29, 2024
@ggwpez ggwpez enabled auto-merge March 9, 2024 15:32
@ggwpez ggwpez added this pull request to the merge queue Mar 9, 2024
Merged via the queue into paritytech:master with commit 1c435e9 Mar 9, 2024
@vikiival vikiival deleted the fix/mint-pallet-attribute branch March 9, 2024 22:00
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
@vikiival @jsidorenko @dharjeezy
🐛 Depositing PalletAttributeSet on incorrect nft (paritytech#2740)
e1ef830 
## Context

Implementing `HolderOf(collection_id)` we have observed a fancy glitch
where pallet deposits event with incorrect values

### Test case 

[Observe following
extrinsic](https://assethub-polkadot.subscan.io/extrinsic/0xdc72321b7674aa209c2f194ed49bd6bd12708af103f98b5b9196e0132dcba777)

To mint in collection `51` user needs to be `HolderOf(50)`.
Therefore current user is owner of item `394` `witness_data {
owned_item: 394 }`

All checking is done correctly, storage is updated correctly

 
![photo_2023-12-18 16 07
11](https://github.com/paritytech/polkadot-sdk/assets/22471030/ca991272-156d-4db1-97b2-1a2873fc5d3f)

However the event which is emitted does not make semantic sense as we
updated storage for `50-394` not for `51-114`

![photo_2023-12-18 16 07
17](https://github.com/paritytech/polkadot-sdk/assets/22471030/c998a92c-e306-4433-aad8-103078140e23)

## The fix 

This PR fixes that depositing `PalletAttributeSet` emits correct values.

---------

Co-authored-by: Jegor Sidorenko <[email protected]>
Co-authored-by: Jegor Sidorenko <[email protected]>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@vikiival @jsidorenko
🐛 Depositing PalletAttributeSet on incorrect nft (paritytech#2740)
4177a3d 
## Context

Implementing `HolderOf(collection_id)` we have observed a fancy glitch
where pallet deposits event with incorrect values

### Test case 

[Observe following
extrinsic](https://assethub-polkadot.subscan.io/extrinsic/0xdc72321b7674aa209c2f194ed49bd6bd12708af103f98b5b9196e0132dcba777)

To mint in collection `51` user needs to be `HolderOf(50)`.
Therefore current user is owner of item `394` `witness_data {
owned_item: 394 }`

All checking is done correctly, storage is updated correctly

 
![photo_2023-12-18 16 07
11](https://github.com/paritytech/polkadot-sdk/assets/22471030/ca991272-156d-4db1-97b2-1a2873fc5d3f)

However the event which is emitted does not make semantic sense as we
updated storage for `50-394` not for `51-114`

![photo_2023-12-18 16 07
17](https://github.com/paritytech/polkadot-sdk/assets/22471030/c998a92c-e306-4433-aad8-103078140e23)

## The fix 

This PR fixes that depositing `PalletAttributeSet` emits correct values.

---------

Co-authored-by: Jegor Sidorenko <[email protected]>
Co-authored-by: Jegor Sidorenko <[email protected]>
bkchr pushed a commit that referenced this pull request Apr 10, 2024
@svyatonik @bkchr
remove no longer valid check from the ensure_weights_are_correct (#2740)
b86ecfe 
* remove no longer valid check from the ensure_weights_are_correct

* fix compilation
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggwpez ggwpez ggwpez approved these changes

+2 more reviewers

@jsidorenko jsidorenko jsidorenko approved these changes

@liamaharon liamaharon liamaharon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T2-pallets This PR/Issue is related to a particular pallet.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vikiival @bkchr @jsidorenko @ggwpez @liamaharon @patriciobcs