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.

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

Expose collection attributes from Inspect trait #1914

Merged
merged 14 commits into from
Oct 26, 2023

Conversation

dastansam
Copy link
Contributor

@dastansam dastansam commented Oct 17, 2023

Description

  • What does this PR do?

While working with pallet_nfts through nonfungibles_v2 traits Inspect, Mutate, I found out that once you have set the collection attribute with <Nfts as Mutate>::set_collection_attribute(), it's not possible to read it with <Nfts as Inspect>::collection_attribute() since they use different namespace values. When setting the attribute, AttributeNamespace::Pallet is used, while AttributeNamespace::CollectionOwner is used when reading.

more context: freeverseio/laos#7 (comment)

This PR makes item an optional parameter in Inspect::system_attribute(), to be able to read collection attributes.

  • Why are these changes needed?

To be able to read collection level attributes when reading attributes of the collection. It will be possible to read collection attributes by passing None for item

  • How were these changes implemented and what do they affect?

NftsApi is also affected and NftsApi::system_attribute() now accepts optional item parameter.

Breaking change

Because of the change in the NftsApi::system_attribute() method's item param, parachains who integrated the NftsApi need to update their API code and frontend integrations accordingly. AssetHubs are unaffected since the NftsApi wasn't released on those parachains yet.

@dastansam
Copy link
Contributor Author

hey @jsidorenko

as we discussed earlier, I am submitting this PR. Do you think I should add tests for this? I don't see any tests for trait implementations, so just want to confirm.

@dastansam dastansam changed the title Ensure collection attributes can be read through traits [draft] Expose collection attributes from Inspect trait Oct 17, 2023
@jsidorenko
Copy link
Contributor

jsidorenko commented Oct 17, 2023

In regards to tests, here is the way to test your changes:

  1. use frame_support::{traits::{tokens::nonfungibles_v2::{Create, Destroy, Inspect, Mutate}}};
assert_ok!(
	<Nfts as Mutate<<Test as SystemConfig>::AccountId, ItemConfig>>::set_collection_attribute(
		&0,
		&[1],
		&[2],
	)
);
assert_eq!(attributes(0), vec![(None, AttributeNamespace::Pallet, bvec![1], bvec![2])]);
assert_eq!(
	<Nfts as Inspect<<Test as SystemConfig>::AccountId>>::system_attribute(&0, None, &[1]),
	Some(vec![2])
);

@jsidorenko jsidorenko added T2-pallets This PR/Issue is related to a particular pallet. T4-runtime_API This PR/Issue is related to runtime APIs. labels Oct 17, 2023
@jsidorenko jsidorenko self-assigned this Oct 17, 2023
@jsidorenko jsidorenko added the R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. label Oct 17, 2023
@dastansam dastansam changed the title [draft] Expose collection attributes from Inspect trait Expose collection attributes from Inspect trait Oct 24, 2023
@dastansam dastansam marked this pull request as ready for review October 24, 2023 15:47
@dastansam dastansam requested review from a team October 24, 2023 15:47
@dastansam
Copy link
Contributor Author

hey @jsidorenko

I added tests and I think it's ready for review

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.

Nice!

@jsidorenko jsidorenko enabled auto-merge (squash) October 26, 2023 09:50
@jsidorenko jsidorenko merged commit 0bcebac into paritytech:master Oct 26, 2023
109 of 110 checks passed
ordian added a commit that referenced this pull request Oct 26, 2023
* master:
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
ordian added a commit that referenced this pull request Oct 26, 2023
* tsv-disabling:
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
ordian added a commit that referenced this pull request Oct 26, 2023
* tsv-disabling: (36 commits)
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
  Refactor candidates test in paras_inherent (#2004)
  PVF: Add worker check during tests and benches (#1771)
  Bump actions/setup-node from 3.8.1 to 4.0.0 (#1997)
  polkadot: enable tikv-jemallocator/unprefixed_malloc_on_supported_platforms (#2002)
  Make `IdentityInfo` generic in `pallet-identity` (#1661)
  Ensure correct variant count in `Runtime[Hold/Freeze]Reason` (#1900)
  `CheckWeight`: Add more logging (#1996)
  ...
s0me0ne-unkn0wn pushed a commit that referenced this pull request Oct 29, 2023
# Description

- What does this PR do?

While working with `pallet_nfts` through `nonfungibles_v2` traits
`Inspect, Mutate`, I found out that once you have set the collection
attribute with `<Nfts as Mutate>::set_collection_attribute()`, it's not
possible to read it with `<Nfts as Inspect>::collection_attribute()`
since they use different `namespace` values. When setting the attribute,
`AttributeNamespace::Pallet` is used, while
`AttributeNamespace::CollectionOwner` is used when reading.

more context:
freeverseio/laos#7 (comment)

This PR makes `item` an optional parameter in
`Inspect::system_attribute()`, to be able to read collection attributes.

- Why are these changes needed?

To be able to read collection level attributes when reading attributes
of the collection. It will be possible to read collection attributes by
passing `None` for `item`

- How were these changes implemented and what do they affect?

`NftsApi` is also affected and `NftsApi::system_attribute()` now accepts
optional `item` parameter.

## Breaking change

Because of the change in the `NftsApi::system_attribute()` method's
`item` param, parachains who integrated the `NftsApi` need to update
their API code and frontend integrations accordingly. AssetHubs are
unaffected since the NftsApi wasn't released on those parachains yet.
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-4-0/5152/1

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
# Description

- What does this PR do?

While working with `pallet_nfts` through `nonfungibles_v2` traits
`Inspect, Mutate`, I found out that once you have set the collection
attribute with `<Nfts as Mutate>::set_collection_attribute()`, it's not
possible to read it with `<Nfts as Inspect>::collection_attribute()`
since they use different `namespace` values. When setting the attribute,
`AttributeNamespace::Pallet` is used, while
`AttributeNamespace::CollectionOwner` is used when reading.

more context:
freeverseio/laos#7 (comment)

This PR makes `item` an optional parameter in
`Inspect::system_attribute()`, to be able to read collection attributes.

- Why are these changes needed?

To be able to read collection level attributes when reading attributes
of the collection. It will be possible to read collection attributes by
passing `None` for `item`

- How were these changes implemented and what do they affect?

`NftsApi` is also affected and `NftsApi::system_attribute()` now accepts
optional `item` parameter.

## Breaking change

Because of the change in the `NftsApi::system_attribute()` method's
`item` param, parachains who integrated the `NftsApi` need to update
their API code and frontend integrations accordingly. AssetHubs are
unaffected since the NftsApi wasn't released on those parachains yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. T2-pallets This PR/Issue is related to a particular pallet. T4-runtime_API This PR/Issue is related to runtime APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants