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.

Sign up for GitHub

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

Jump to bottom

🐛 Depositing PalletAttributeSet on incorrect nft #2740

Merged
merged 9 commits into from
Mar 9, 2024
Merged

🐛 Depositing PalletAttributeSet on incorrect nft #2740

merged 9 commits into from
Mar 9, 2024

Conversation

vikiival
Copy link
Contributor

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-silent Changes should not be mentioned in any release notes 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

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 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

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

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

@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
128 of 130 checks passed
@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

@liamaharon liamaharon liamaharon approved these changes

@ggwpez ggwpez ggwpez approved these changes

@jsidorenko jsidorenko jsidorenko approved these changes

Assignees
No one assigned
Labels
R0-silent Changes should not be mentioned in any release notes 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.
5 participants
@vikiival @bkchr @jsidorenko @ggwpez @liamaharon