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

Fix broken pallet_shards benchmarks #1017

Merged
merged 12 commits into from
Jul 22, 2024

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Jul 19, 2024

Closes #1016

shards pallet fixes

  • split UnexpectedCommit error into each possible error branch, 2 branches: (1) return UnexpectedCommit error iff commit is not expected to be called because of the member's current status not equalling MemberStatusAdded (2) return MemberPeerIdNotFound error iff the member has not been registered via the members pallet prior to calling commit
  • replaced non-generic pallet_shards::Config direct inheritance requirements with generic requirements that match all other custom pallets: frame_system::Config<AccountId = AccountId> + pallet_balances::Config<Balance = Balance>

All of these changes were required by the benchmarking fixes in this PR (listed further below).

shards pallet unit test fixes

  • impl pallet_members::Config for pallet_shards::mock::Test
  • configure new Members instance as Members config for pallet_shards::mock::Test impl of pallet_shards::Config
  • update all pallet-shards unit tests s.t. they pass after the above changes to pallet_shards::mock (requires registering member prior to every call to commit)

shards pallet benchmarking fixes

  • commit
  • ready

@4meta5 4meta5 added bug Something isn't working WIP labels Jul 19, 2024
@4meta5 4meta5 self-assigned this Jul 19, 2024
@4meta5 4meta5 added ready-for-review PR author(s) done with coding and reviewer[s] can start priority: critical !ci-benchmark Benchmark and commit new weights and removed WIP labels Jul 20, 2024
@haider-rs
Copy link
Contributor

seems to give a compilation error after geenrating weights:

use polkadot_sdk::*;

any idea why this import was removed from the weight.rs file?

@4meta5
Copy link
Contributor Author

4meta5 commented Jul 21, 2024

@penumbra23 do you know why the import was removed when the weight machine pushed the weights?

Copy link
Contributor

@haider-rs haider-rs left a comment

Choose a reason for hiding this comment

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

looking good!

@4meta5 4meta5 removed the !ci-benchmark Benchmark and commit new weights label Jul 22, 2024
@4meta5 4meta5 merged commit 3f5f16d into development Jul 22, 2024
16 checks passed
@haider-rs haider-rs deleted the amar-fix-failing-shard-benchmarks branch July 29, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: critical ready-for-review PR author(s) done with coding and reviewer[s] can start
Projects
None yet
Development

Successfully merging this pull request may close these issues.

broken pallet_shards benchmarks
2 participants