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

Polkadot People Chain #319

Merged

Conversation

joepetrowski
Copy link
Contributor

@joepetrowski joepetrowski commented May 21, 2024

Polkadot People Chain

To verify prior to release: People Chain poke_deposit weight must be less than 2x Relay's. E.g.:

>>> weight_per_nano = 1_000
>>> relay_read = 20_499 * weight_per_nano
>>> relay_write = 83_471 * weight_per_nano
>>> para_read = 25_000 * weight_per_nano
>>> para_write = 100_000 * weight_per_nano
>>>
>>> def relay():
...     time = 145_781_000 + (3 * relay_read) + (3 * relay_write)
...     pov = 11_037
...     return (time, pov)
...
>>> def para():
...     time = 52_060_000 + (3 * para_read) + (3 * para_write)
...     pov = 6_723
...     return (time, pov)
...
>>>
>>> (relay_time, relay_pov) = relay()
>>> (para_time, para_pov) = para()
>>>
>>> (2 * relay_time) > para_time
True
>>> (2 * relay_pov) > para_pov
True

@joepetrowski joepetrowski mentioned this pull request May 21, 2024
7 tasks
Copy link

Review required! Latest push from author must always be reviewed

Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

Looking ready, just a few small things below. people-polkadot is also missing from runtimes-matrix.json.

I also didn't check the weights as they seem to be unchanged from people-kusama. Will you run the benchmarks in this PR or elsewhere? Would be a good check to make sure the factor of two is sufficient between relay and para in case the ratio is very different on Polkadot

@github-actions github-actions bot requested a review from seadanda May 30, 2024 08:25
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

I found there are too many duplicated code for a new system parachain. This is a code smell and should be addressed at some stage. We will have more system parachains and we need to reduce the overhead of making one before making more system parachains.

if !self.legal.is_none() {
res.insert(IdentityField::Legal);
}
if !self.web.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if !self.web.is_none() {
if self.web.is_some() {

I assume that this is copied from elsewhere, so no big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just from the Kusama config. Just idiomatic or is there an advantage to is_some?

Copy link
Member

Choose a reason for hiding this comment

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

No real difference - just readability.

@joepetrowski
Copy link
Contributor Author

/merge

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) June 13, 2024 12:00
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

Copy link
Contributor

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Just approving to get ci happy

@fellowship-merge-bot fellowship-merge-bot bot merged commit 329ceca into polkadot-fellows:main Jun 13, 2024
39 of 40 checks passed
@joepetrowski joepetrowski deleted the polkadot-people branch June 13, 2024 13:05
@seadanda
Copy link
Contributor

I found there are too many duplicated code for a new system parachain. This is a code smell and should be addressed at some stage. We will have more system parachains and we need to reduce the overhead of making one before making more system parachains.

@xlc would be good to get your input on paritytech/polkadot-sdk#4815 as a first step towards minimising the diff between system parachains

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.

7 participants