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

client refactor #1263

Merged
merged 5 commits into from
Jun 18, 2024
Merged

client refactor #1263

merged 5 commits into from
Jun 18, 2024

Conversation

ermalkaleci
Copy link
Contributor

@ermalkaleci ermalkaleci commented Jun 17, 2024

  • It was possible to remove #[cfg(feature = "evm-tracing")] for local service so did that. All featured related code move into a single start_node method. Although this is not possible for start_node_impl but I was able to remove duplicated for start_astar_node, start_shiden_node, start_shibuya_node.
  • Old build_import_queue is renamed to build_import_queue_fallback used by Shiden (see code comments).
  • New build_import_queue will create aura block importer to be used for Astar and Shibuya.
  • Remove deprecated aura stuff and instead create a general purpose block authoring which will switch to aura consensus when it becomes available. This is required for Shiden only because Astar and Shibuya will start with Aura

@ermalkaleci ermalkaleci added the client This PR/Issue is related to the topic “client”. label Jun 17, 2024
Copy link

Code Coverage

Package Line Rate Branch Rate Health
pallets/astar-xcm-benchmarks/src 88% 0%
precompiles/dapp-staking-v3/src 90% 0%
primitives/src/xcm 64% 0%
chain-extensions/xvm/src 0% 0%
pallets/static-price-provider/src 52% 0%
pallets/inflation/src 83% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
pallets/collator-selection/src 92% 0%
precompiles/dispatch-lockdrop/src 86% 0%
chain-extensions/types/unified-accounts/src 0% 0%
precompiles/sr25519/src 64% 0%
pallets/dynamic-evm-base-fee/src 92% 0%
primitives/src 61% 0%
pallets/dapp-staking-v3/src/test 0% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
pallets/ethereum-checked/src 79% 0%
precompiles/substrate-ecdsa/src 74% 0%
pallets/xc-asset-config/src 64% 0%
chain-extensions/types/assets/src 0% 0%
chain-extensions/types/xvm/src 0% 0%
precompiles/unified-accounts/src 100% 0%
chain-extensions/unified-accounts/src 0% 0%
precompiles/xvm/src 75% 0%
pallets/dapp-staking-v3/src 92% 0%
precompiles/assets-erc20/src 81% 0%
chain-extensions/pallet-assets/src 56% 0%
pallets/dapp-staking-v3/src/benchmarking 98% 0%
pallets/xvm/src 54% 0%
precompiles/xcm/src 73% 0%
pallets/dapp-staking-migration/src 0% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
pallets/unified-accounts/src 86% 0%
pallets/oracle-benchmarks/src 0% 0%
pallets/price-aggregator/src 72% 0%
Summary 78% (3600 / 4635) 0% (0 / 0)

Minimum allowed line rate is 50%

Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

1 more code comment, otherwise LGTM

For the PR summary, thanks for not leaving it empty but unfortunately it's not informative at all. It doesn't help with code review, so it would be the same even if it was empty.

I'd suggest to summarize the changes you do, especially for PRs such as these where lots of code is moved or deleted. That would make review easier, and would help set the tone for what to focus on.

E.g.:

  • removed duplicated start_node_impl by using feature flag internally instead on the function level
  • ...

bin/collator/src/parachain/service.rs Show resolved Hide resolved
@ermalkaleci
Copy link
Contributor Author

1 more code comment, otherwise LGTM

For the PR summary, thanks for not leaving it empty but unfortunately it's not informative at all. It doesn't help with code review, so it would be the same even if it was empty.

I'd suggest to summarize the changes you do, especially for PRs such as these where lots of code is moved or deleted. That would make review easier, and would help set the tone for what to focus on.

E.g.:

  • removed duplicated start_node_impl by using feature flag internally instead on the function level
  • ...

how that I am looking at "Files changed" there are a lot of changes. I will update to explain everything in more details

Copy link
Member

@ashutoshvarma ashutoshvarma left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for reducing 999 lines of redundant code :)

@ermalkaleci ermalkaleci merged commit 5477e6f into master Jun 18, 2024
9 checks passed
@ermalkaleci ermalkaleci deleted the refactor/client branch June 18, 2024 09:39
ermalkaleci added a commit that referenced this pull request Jun 18, 2024
ermalkaleci added a commit that referenced this pull request Jun 18, 2024
* Revert "client refactor (#1263)"

This reverts commit 5477e6f.

* revert upload-artifacts to v3
ermalkaleci added a commit that referenced this pull request Jun 18, 2024
Dinonard pushed a commit that referenced this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client This PR/Issue is related to the topic “client”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants