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] Update Frequency to v0.9.30 #744

Merged
merged 26 commits into from
Dec 13, 2022
Merged

Conversation

saraswatpuneet
Copy link
Collaborator

@saraswatpuneet saraswatpuneet commented Dec 5, 2022

Goal

The goal of this PR is to upgrade substrate to polkadot release .9.30

Closes paritytech/substrate#704

Notes:

PR for Substrate upgrade to 0.9.30
Release Notes

Major updates from 0.9.29 to 0.9.30:

Checklist

  • n/a Chain spec updated
  • n/a Custom RPC OR Runtime API added/changed? Updated js/api-augment.
  • n/a Design doc(s) updated
  • n/a Tests added
  • n/a Benchmarks added
  • Weights updated
  • Run benchmarks

@demisx
Copy link
Collaborator

demisx commented Dec 5, 2022

Per my Slack message, I recommend test running all 3 CI workflows before merging #744 to main, so there are now surprises later. Let me know if you need my help with it when you are ready to merge.

@saraswatpuneet saraswatpuneet changed the title Chore/upgrade 0.9.32 Chore/upgrade 0.9.30 Dec 5, 2022
@saraswatpuneet saraswatpuneet marked this pull request as ready for review December 5, 2022 20:54
Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

It's a nitpick, but it would help with reviewing if you could summarize the changes involved in the upgrade, so I can know what to expect and review more efficiently. There was a lot of renaming involved, it looks like, as well as some parameter additions and reordering in cumulus.

@@ -339,6 +339,15 @@ pub fn run() -> Result<()> {
)?;
cmd.run(partials.client)
}),
#[cfg(not(feature = "runtime-benchmarks"))]
frame_benchmarking_cli::BenchmarkCmd::Storage(_) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this required for the upgrade to v0.9.30?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

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 was a possible panic when someone was running storage benchmarks without the flags, but since template node has updated it so has others

@@ -308,7 +308,7 @@ where
let prometheus_registry = parachain_config.prometheus_registry().cloned();
let transaction_pool = params.transaction_pool.clone();
let import_queue = cumulus_client_service::SharedImportQueue::new(params.import_queue);
let (network, system_rpc_tx, start_network) =
let (network, system_rpc_tx, tx_handler_controller, start_network) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: If these parameter changes are part of the update it would be nice to note it. I would prefer not to assume so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes these are changes in the underlying apis

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This update is interesting its a step towards separating collator with embedded relay paritytech/cumulus#1585

@@ -88,7 +88,7 @@ pub mod pallet {
#[pallet::config]
pub trait Config: frame_system::Config {
/// The overarching event type.
type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this also renamed in the new substrate version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #744 (ed504d7) into main (2c39942) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     paritytech/substrate#744   +/-   ##
=======================================
  Coverage   79.58%   79.58%           
=======================================
  Files          19       19           
  Lines         823      823           
=======================================
  Hits          655      655           
  Misses        168      168           
Impacted Files Coverage Δ
pallets/messages/src/lib.rs 86.81% <ø> (ø)
pallets/msa/src/lib.rs 84.22% <ø> (ø)
pallets/schemas/src/lib.rs 80.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@saraswatpuneet saraswatpuneet changed the title Chore/upgrade 0.9.30 [CHORE] Update Frequency to v0.9.30 Dec 5, 2022
Copy link
Collaborator

@aramikm aramikm left a comment

Choose a reason for hiding this comment

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

Added some questions but overall looks good.
In Rust compiler We Trust

.github/workflows/merge-pr.yml Show resolved Hide resolved
@@ -13,7 +13,7 @@
"author": "",
"license": "Apache-2.0",
"dependencies": {
"@frequency-chain/api-augment": "v0.9.29",
"@frequency-chain/api-augment": "v0.9.30",
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 this going to affect metadata for other libraries?

Copy link
Collaborator Author

@saraswatpuneet saraswatpuneet Dec 6, 2022

Choose a reason for hiding this comment

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

I think exacting the version might complicate this we should do "^v0.9.29" for now given I am not sure at what stage these tests are run but for sure npm wont have this version till we release

Copy link
Collaborator

@demisx demisx left a comment

Choose a reason for hiding this comment

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

Should each "Checklist" item of this PR needs to be checked off?

Screen Shot 2022-12-05 at 4 50 52 PM

@saraswatpuneet saraswatpuneet added blocked Blocked by another issue and removed blocked Blocked by another issue labels Dec 8, 2022
@wilwade wilwade added the change/major Major Changes in this PR label Dec 13, 2022
Copy link
Collaborator

@wilwade wilwade left a comment

Choose a reason for hiding this comment

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

Only one thing that squeaked in that must change, but a few other questions and I think we want to run benchmarks before merging.

Comment on lines 16 to 19
"@frequency-chain/api-augment": "^v0.9.29",
"@polkadot/api": "^9.6.2",
"@polkadot/types": "^9.6.2",
"@polkadot/util": "^8.7.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a reversion.

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 did this to pin the version on which integration tests run fine

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 see yes something updated since this PR let me keep whats in main

node/service/src/service.rs Show resolved Hide resolved
@@ -570,7 +569,7 @@ where
"Failed to create parachain inherent",
)
})?;
Ok((time, slot, parachain_inherent))
Ok((slot, timestamp, parachain_inherent))
Copy link
Collaborator

Choose a reason for hiding this comment

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

These order swaps worry me. It feels like a mistake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes this is change observed in the way they have modified starting node due to upcoming changes

Here is a boilerplate PR https://github.com/substrate-developer-hub/substrate-parachain-template/pull/133

Its a change across substrate

@@ -91,6 +91,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Standard Error: 139_000
.saturating_add(Weight::from_ref_time(4_711_000 as u64).saturating_mul(s as u64))
.saturating_add(T::DbWeight::get().reads(1 as u64))
.saturating_add(T::DbWeight::get().writes(2 as u64))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did these come from?

Also we should likely run benchmarks before merging right? Lots of weights could/should have changed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Older weights than Main , need to run benchmarks before merge to update these

@@ -122,6 +122,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Standard Error: 6_000
.saturating_add(Weight::from_ref_time(1_193_000 as u64).saturating_mul(s as u64))
.saturating_add(T::DbWeight::get().reads(2 as u64))
.saturating_add(T::DbWeight::get().writes(5 as u64))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another weights update I didn't expect.

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 think these are just older weights, I need to run the benchmarks on this PR

@@ -157,7 +164,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("frequency"),
impl_name: create_runtime_str!("frequency"),
authoring_version: 1,
spec_version: 3,
spec_version: 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙇

@@ -13,7 +13,7 @@
"author": "",
"license": "Apache-2.0",
"dependencies": {
"@frequency-chain/api-augment": "v0.9.29",
"@frequency-chain/api-augment": "^v0.9.29",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is mostly done as .9.30 wont be available until release, so pinning on last stable release

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to make it so that for CI this builds and uses the "local" version so they stay in sync for ci. On my need a break list ;)

Copy link
Collaborator

@wilwade wilwade left a comment

Choose a reason for hiding this comment

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

Approved (benchmarks are running)

@saraswatpuneet saraswatpuneet merged commit da4426f into main Dec 13, 2022
@saraswatpuneet saraswatpuneet deleted the chore/upgrade_0.9.32 branch December 13, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change/major Major Changes in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants