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

Fix region nonfungible implementation #5067

Merged
merged 14 commits into from
Jul 26, 2024
Merged

Fix region nonfungible implementation #5067

merged 14 commits into from
Jul 26, 2024

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Jul 18, 2024 •
edited
Loading

The problem with the current implementation is that minting will cause the region coremask to be set to Coremask::complete regardless of the actual coremask.

This PR fixes that.

More details about the nonfungible implementation can be found here: #3455

@Szegoo Szegoo requested a review from a team as a code owner July 18, 2024 15:21
seadanda
seadanda reviewed Jul 18, 2024
View reviewed changes
Copy link
Contributor

@seadanda seadanda 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.

Oof, good catch. Would be nice to add a test to interlace or partition->burn->mint and then check the mask is identical after minting. This PR should also be backported to v1.14

@seadanda seadanda added the T2-pallets This PR/Issue is related to a particular pallet. label Jul 18, 2024
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 18, 2024 15:53
@Szegoo
Copy link
Contributor Author

Szegoo commented Jul 18, 2024

Would be nice to add a test to interlace or partition->burn->mint and then check the mask is identical after minting.

I added two new tests. They don't explicitly check for the coremask, but if the coremask changed when minting, the region returned would be the old one which still has the owner set to None, so the test would fail without this fix.

@seadanda
Copy link
Contributor

Looks good, just needs a prdoc. I just realised it's breaking as issue is pub, so it might be an issue to backport.

@Szegoo
Copy link
Contributor Author

Szegoo commented Jul 18, 2024

I just realised it's breaking as issue is pub, so it might be an issue to backport.

But it is not exposed as an extrinsic; it can only be accessed from the runtime code, right?

@Szegoo Szegoo requested a review from seadanda July 23, 2024 06:45
seadanda
seadanda reviewed Jul 23, 2024
View reviewed changes
substrate/frame/broker/src/utility_impls.rs Show resolved Hide resolved
prdoc/pr_5067.prdoc Outdated Show resolved Hide resolved
@Szegoo @seadanda
Update prdoc/pr_5067.prdoc
49c77a0 
Co-authored-by: Dónal Murray <[email protected]>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 23, 2024 10:33
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6779083

@ggwpez
Copy link
Member

ggwpez commented Jul 23, 2024

The semver CI check should be fixed no master, please try again after merging it in.

@seadanda seadanda requested a review from bkontur July 24, 2024 08:43
@seadanda seadanda self-assigned this Jul 24, 2024
bkontur
bkontur approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

lgtm, just nits with import (can be also ignored)

cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs Outdated Show resolved Hide resolved
cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs Outdated Show resolved Hide resolved
cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs Outdated Show resolved Hide resolved
@seadanda seadanda enabled auto-merge July 24, 2024 15:38
@seadanda seadanda added this pull request to the merge queue Jul 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2024
@Szegoo
Copy link
Contributor Author

Szegoo commented Jul 25, 2024 •
edited
Loading

The semver check was failing. Maybe merging with the master branch again will fix it?

@ggwpez ggwpez enabled auto-merge July 26, 2024 17:27
@ggwpez ggwpez added this pull request to the merge queue Jul 26, 2024
Merged via the queue into paritytech:master with commit c39cc33 Jul 26, 2024
159 of 161 checks passed
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
@Szegoo @seadanda
@bkontur @franciscoaguirre @ggwpez @TarekkMA
Fix region nonfungible implementation (paritytech#5067)
1a31140 
The problem with the current implementation is that minting will cause
the region coremask to be set to `Coremask::complete` regardless of the
actual coremask.

This PR fixes that.

More details about the nonfungible implementation can be found here:
paritytech#3455

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez mentioned this pull request Sep 9, 2024
Closed
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seadanda seadanda seadanda approved these changes

@bkontur bkontur bkontur approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

Assignees

@seadanda seadanda

Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Security Audit (PRs) - SRLabs
Status: Audited
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Szegoo @seadanda @paritytech-cicd-pr @ggwpez @franciscoaguirre @bkontur