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

chore: upgrade to polkadot v0.9.28 #564

Merged
merged 85 commits into from Oct 22, 2022
Merged

chore: upgrade to polkadot v0.9.28 #564

merged 85 commits into from Oct 22, 2022

Conversation

Roznovjak
Copy link
Collaborator

@Roznovjak Roznovjak commented Sep 29, 2022

Notable changes:
Rename: Class -> Collection, Instance -> Item
Use our own copy of orml_currencies pallet from warehouse
New benchmarking commands (node/src/command.rs)
No need for #[transactional] macro
Implemented migration for pallet_nft in warehouse
Implemented migration for pallet_marketplace
Added migration for pallet_uniques
The version of fixed crate fixed to 1.17.0
Use polkadot with patched XCM executor
Add cumulus_pallet_xcmp_queue to benchmarking

New or updated runtime configuration parameters:
orml_tokens::MaxReserves = 50
orml_tokens::ReserveIdentifier = ()
pallet_currencies::CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases
pallet_elections_phragmen::MaxCandidated = 1_000
pallet_elections_phragmen::MaxVoters = 10_000
pallet_uniques::CreateOrigin = NeverEnsureOrigin
pallet_uniques::Locker = ()
cumulus_pallet_xcmp_queue::WeightInfo = ()
orml_xtokens::MultiLocationsFilter = Everyting
orml_xtokens::ReserveProvider = AbsoluteReserveProvider
orml_xtokens::MinXcmFee = ParachainMinFee

Copy link
Contributor

@dmoka dmoka left a comment

Choose a reason for hiding this comment

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

Apart from some small details, LGTM.

It would be nice if someone else could also look at this PR before merging back to master.

Cargo.toml Outdated Show resolved Hide resolved
node/src/service.rs Outdated Show resolved Hide resolved
pallets/duster/src/mock.rs Show resolved Hide resolved
pallets/marketplace/src/migration.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@apopiak
Copy link
Collaborator

apopiak commented Oct 7, 2022

XcmConfig::WeightInfo = ()
So we didn't add benchmarks for XCM?

@Roznovjak
Copy link
Collaborator Author

XcmConfig::WeightInfo = ()
So we didn't add benchmarks for XCM?

Where did you find this line? I can't find it. XcmConfig doesn't have this field.

@apopiak
Copy link
Collaborator

apopiak commented Oct 7, 2022

XcmConfig::WeightInfo = ()
So we didn't add benchmarks for XCM?

Where did you find this line? I can't find it. XcmConfig doesn't have this field.

From your PR description up above :-D
Edit: Looks like you might have meant the cumulus_pallet_xcmp_queue?

@Roznovjak
Copy link
Collaborator Author

XcmConfig::WeightInfo = ()
So we didn't add benchmarks for XCM?

Where did you find this line? I can't find it. XcmConfig doesn't have this field.

From your PR description up above :-D Edit: Looks like you might have meant the cumulus_pallet_xcmp_queue?

Yes, you are right. I will update the description. And yes, we don't have benchmarks for it. @enthusiastmartin what to do if we want to add benchmarks for a pallet that we haven't benchmarked before? Is it enough to just add new file with the default weights to the common_runtime::weights?

Cargo.toml Outdated Show resolved Hide resolved
node/src/cli.rs Show resolved Hide resolved
Comment on lines +95 to +96
let _ = <ExchangeAssetsIntentionCount<T>>::clear(u32::MAX, None);
let _ = <ExchangeAssetsIntentions<T>>::clear(u32::MAX, None);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is ExchangeAssetsIntentions bounded?
Not the purpose of this PR, but we should know and it should be documented IMO

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't need to block merging this PR I guess as it's a different concern, but we should have a task tracking this IMO

pallets/marketplace/src/lib.rs Show resolved Hide resolved
Comment on lines +439 to +442
// The latest versions of the orml-currencies pallet don't emit events.
// The infrastructure relies on the events from this pallet, so we use the latest version of
// the pallet that contains and emit events and was updated to the polkadot version we use.
impl pallet_currencies::Config for Runtime {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused, I saw the events being removed from some tests, no?

Comment on lines -144 to -150
orml_currencies::Event::Transferred {
currency_id,
from: *ALICE,
to: *TREASURY,
amount: 500,
}
.into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a comment in the runtime that orml_currencies was forked to keep these events, but they seem to be removed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed orml events that followed events from the balances or tokens pallets. So we test that transfers are executed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, so pallet_currencies keeps some but not all events?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It keeps all events, but I removed events from tests if there was an event from balances or tokens corresponding to the same transfer. A transfer emits two events, one from balances/tokens and one from currencies

runtime/basilisk/src/lib.rs Show resolved Hide resolved
runtime/basilisk/src/migrations.rs Show resolved Hide resolved
@apopiak
Copy link
Collaborator

apopiak commented Oct 11, 2022

What to do if we want to add benchmarks for a pallet that we haven't benchmarked before?

They need to be added in the benchmark listing in the runtime lib.rs, no?

Copy link
Contributor

@enthusiastmartin enthusiastmartin left a comment

Choose a reason for hiding this comment

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

resolve all comments first.

@Roznovjak Roznovjak merged commit 6c242fe into master Oct 22, 2022
@jak-pan jak-pan deleted the polkadot-v0.9.28 branch February 22, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants