Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

paras: fix upgrade restriction signal #4603

Merged
merged 1 commit into from
Dec 31, 2021

Conversation

pepyakin
Copy link
Contributor

Closes #3971

Read the linked issue.

Apart from that, this addresses the concern raised in this comment by
just adding a test. I couldn't find a clean way to reconcile a block
number delay with a PVF voting TTL, so I just resorted to rely on the
test. Should be fine for now.

@pepyakin
Copy link
Contributor Author

pepyakin commented Dec 24, 2021

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Dec 24, 2021
@pepyakin pepyakin added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Dec 24, 2021
@pepyakin pepyakin force-pushed the pep-configuration-go-ahead-consistency branch from 609c38a to 3a94ef4 Compare December 27, 2021 14:25
@pepyakin pepyakin force-pushed the pep-configuration-go-ahead-consistency branch from 3a94ef4 to f058f30 Compare December 28, 2021 13:57
Base automatically changed from pep-configuration-go-ahead-consistency to master December 28, 2021 15:46
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Honestly I don't see an issue with #3971 ... but I also don't know how we handle such things in general. I assume this is to make things consistent with other code?

runtime/parachains/src/configuration.rs Outdated Show resolved Hide resolved
Closes #3971

Read the linked issue.

Apart from that, this addresses the concern raised in this [comment] by
just adding a test. I couldn't find a clean way to reconcile a block
number delay with a PVF voting TTL, so I just resorted to rely on the
test. Should be fine for now.

[comment]: #4457 (comment)
@@ -1004,7 +1004,9 @@ impl<T: Config> Pallet<T> {
}

/// Called by the initializer to finalize the configuration pallet.
Copy link
Member

Choose a reason for hiding this comment

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

Not your fault, but the comment is c&p?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but it does look correct, or?

Copy link
Member

Choose a reason for hiding this comment

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

Is this the configuration pallet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah gotcha xD will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pepyakin added a commit that referenced this pull request Dec 31, 2021
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
#4603 (comment)
pepyakin added a commit that referenced this pull request Dec 31, 2021
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
#4603 (comment)
@pepyakin
Copy link
Contributor Author

It is not critical by any means, but it is an issue, I believe. Fixing it will actually simplify the behavior from my perspective (at the cost of small complication because of having to do logic in on_finalize) and that will less likely shoot us in the foot down the road

@pepyakin
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 9b4ea01 into master Dec 31, 2021
@paritytech-processbot paritytech-processbot bot deleted the pep-paras-restriction branch December 31, 2021 15:32
pepyakin added a commit that referenced this pull request Jan 1, 2022
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
#4603 (comment)
pepyakin added a commit that referenced this pull request Jan 1, 2022
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
#4603 (comment)
ordian added a commit that referenced this pull request Jan 3, 2022
* master:
  Set CurrentCodeHash before running some dispatchable benchmarks (#4645)
  paras: split tests (#4636)
  Bump quote from 1.0.10 to 1.0.14 (#4632)
  Bump pin-project from 1.0.8 to 1.0.9 (#4606)
  chore: fix copy&paste and tidy comments (#4646)
  derive Copy and Clone for Upgrade signals (#4637) (#4647)
  paras: fix upgrade restriction signal (#4603)
  configuration: Rename validation_upgrade_{frequency -> cooldown} (#4635)
  Bump lru from 0.7.1 to 0.7.2 (#4633)
  paras: add governance control dispatchables (#4575)
drahnr pushed a commit that referenced this pull request Jan 4, 2022
Closes #3971

Read the linked issue.

Apart from that, this addresses the concern raised in this [comment] by
just adding a test. I couldn't find a clean way to reconcile a block
number delay with a PVF voting TTL, so I just resorted to rely on the
test. Should be fine for now.

[comment]: #4457 (comment)
drahnr pushed a commit that referenced this pull request Jan 4, 2022
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
#4603 (comment)
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
Closes paritytech#3971

Read the linked issue.

Apart from that, this addresses the concern raised in this [comment] by
just adding a test. I couldn't find a clean way to reconcile a block
number delay with a PVF voting TTL, so I just resorted to rely on the
test. Should be fine for now.

[comment]: paritytech#4457 (comment)
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
paritytech#4603 (comment)
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Mar 10, 2023
As was brought up in [here][og], we have some copy&paste comments. I
fixed them and also took liberty of fixing some other places.
Specifically, those that say "module" instead of contemporary "pallet".

[og]:
paritytech/polkadot#4603 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UpgradeRestrictionSignal is one block off
3 participants