Skip to content

Conversation

@4meta5
Copy link
Contributor

@4meta5 4meta5 commented Apr 18, 2021

What does it do?

adds a builder pattern for parachain-staking test externalities, instead of hiding the config behind functions

This is just like in #339 CC @JoshOrndorff

Improves readability

What important points reviewers should know?

Is there something left for follow-up PRs?

make genesis more readable #363 (comment)

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Checklist

  • Does it require a purge of the network?
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ?

@4meta5 4meta5 added A0-pleasereview Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Apr 18, 2021
@4meta5
Copy link
Contributor Author

4meta5 commented Apr 18, 2021

This makes me think I should change the parachain-staking genesis config from

pub struct GenesisConfig {
  ..
  stakers: Vec<(AccountId, Option<AccountId>, Balance)>
}

to

pub struct GenesisConfig {
  ..
  collators: Vec<(AccountId, Balance)>,
  nominators: Vec<(AccountId, AccountId, Balance)>,
}

This is more of a breaking change, but I do think it is objectively better.

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

i think this is a good change.

The diff is huge mostly becaue of the tests.rs file. Is this just the result of indentation changing because of the new builder style? Or have the tests actually changed?

@4meta5
Copy link
Contributor Author

4meta5 commented Apr 19, 2021

Is this just the result of indentation changing because of the new builder style? Or have the tests actually changed?

Yes, the builder style caused lots of indentation changes because x_collators_y_nominators() functions were replaced with the specific ExtBuilder::default().with_collators().with_nominators().build()

And No, none of the tests changed at all otherwise

@4meta5 4meta5 merged commit 83ad0c0 into master Apr 19, 2021
@4meta5 4meta5 deleted the amar-stake-test-ext branch April 19, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A0-pleasereview Pull request needs code review. B0-silent Changes should not be mentioned in any release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants