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

chore: update polkadot sdk 1.15.2, rust to 2024-07-31 #5306

Merged
merged 28 commits into from
Oct 9, 2024

Conversation

syan095
Copy link
Contributor

@syan095 syan095 commented Oct 1, 2024

Pull Request

Closes: PRO-1407

Summary

Updated to PolkadotSdk version 1.15.2 (almost) the latest release.
Updated Rustc version to nightly-2024-07-31. The next version after that generates warnings for "unreachable-pattern" within Substrate's pallet::call macro.

Major changes and TODOs to review is listed below. I have numbered them so they can be easily referred to. I would suggest we split some of them into separate linear tasks that can be done progressively after this upgrade.

  1. subcommand::try-runtime is deprecated

  2. There's a way to change pallet const parameters at Runtime through the use of extrinsic pallet_parameters::set_parameter.
    TODO: Do we want to use this feature?

  3. Runtime V2: new proc. macro for constructing Runtime.
    TODO: Review:

    • All pallets have assigned pallet_index.
    • Current pallet indexes are continuous and ungrouped.
    • should we better group the pallets? (leave gaps in indexes)
  4. try-runtime-cli: fully deprecated. Use paritytech/try-runtime-cli instead

  5. Consider to use workspace dependencies to unify dependency versions so they are consistent across all packages.
    TODO: Consider unify dependency versions

  6. TODO: Review - can this be removed?
    #[allow(dead_code)]
    fn input_to_output_amount_floor(amount: Amount, tick: Tick) -> Option;

  7. Deprecated function itertools::unfold
    Replaced with itertools::unfold(|t| {...}

  8. Runtime sp_genesis_builder::GenesisBuilder<Block> now requires fn preset_names()
    TODO: review if we need to use this feature.
    Overhaul in how "Presets" can be queried.

  9. Runtime: System::Config now has extra fields:
    TODO review if () is OK. Review if we want to refactor all mock Runtimes to use v2 macro.
    type SingleBlockMigrations = ();
    type MultiBlockMigrator = ();
    type PreInherents = ();
    type PostInherents = ();
    type PostTransactions = ();

  10. Aura now need SlotDuration defined explicitly.
    Currently set to SLOT_DURATION (1 block = 6000ms)
    TODO: Review

  11. new_full: sc_consensus_grandpa::grandpa_peers_set_config::<_, Network> now has 2 additional fields, metrics and peer_store_handle

    • now require additional type: Libp2p or Litep2p
  12. Jsonrpsee is updated to 0.23.2. Major changes on error types

  • Error is now ErrorObjectOwned, which contains code, message and data
  • TODO: review:
    • RPC output encoding/serialization
    • Use of error code CALL_EXECUTION_FAILED_CODE
    • cf_subscribe_scheduled_swaps no long return error
    • new_subscription no longer return error.
    • new_subscription is not async,
      ! on-success: uses substrate version of pipe_from_stream
      ! on-fail: spawn a new thread to "reject" the sink.
    • subscription error now wrapped by ErrorObjectOwned
  1. We return anyhow::error for Lots of rpc functions. They are currently converted to ErrorOjbectOwned via to_rpc_error
    TODO: review if we should update them to return jsonrpsee::RpcResult instead.

  2. TODO: async fn should_update_version_on_bad_proof() Subscription::new() is now private - the specific test case has been commented out for now, this needs to be fixed/replaced/removed.
    Also Mockall is having trouble creating types that don't have "default" impl. Mockall may need to be replaced here.
    The entire file is commented out right now
    (Jamie said he might pick this up)

  3. Statechain observer:
    - updated how RPC errors are being matched.

  4. Lots of error conversion in LP APIs, Broker API, ChainCli, Engine. All anyhow::error are converted into ErrorObjectOwned via to_rpc_error. TODO discuss return types (I think it would be easier to always return anyhow::result and map_err() into jsonrpsee error, in case jsonrpsee changes its error type again.

  5. subscribe_order_fills changed - SubscriptionSinks no longer need to be closed manually.

  6. Clap changes: "version_short" removed, auto set to "-V"

  • #[clap(parse(from_os_str))] -> #[clap(value_parser(clap::value_parser!(PathBuf)))]
  1. Runtime v2 now has "GenesisConfig build test", where all pallets' genesisconfig is built using GenesisConfig::default().
    This is causing test fail in cases such as min_fun = redeption_tax = 0, or when
    initial_authorities = []. Asserts are replaced with frame_support::print but return a value to ensure this test will pass.
    I updated the "Default" impl in these cases to make tests pass. This is OK assuming GenesisConfig::Defualt is NEVER used in Production.
    TODO: Review if this is OK.

  2. Note: Builds with runtime-benchmarking cannot run. For building local net, the benchmkaring feature must be turned off.

  3. TODO: In the next rust version(2024-08-14), "unreachable-pattern" will trigger rust compiler warning. We should review all cases of "unreachable-pattern" (with the unreachable!() macro) and review them

  4. Getting lint clippy::thread_local_initializer_can_be_made_consthas been renamed toclippy::missing_const_for_thread_local` clippy warning. We don't use this, not sure why we are getting this warning for all our build packages

@syan095 syan095 requested review from kylezs, msgmaxim, dandanlen and a team as code owners October 1, 2024 09:33
@syan095 syan095 requested review from jerryafr, acdibble and j4m1ef0rd and removed request for a team October 1, 2024 09:33
@syan095 syan095 changed the title chore: update polkadot sdk 1.15.2, rust to 2024-08-04 chore: update polkadot sdk 1.15.2, rust to 2024-07-31 Oct 2, 2024
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 25.07289% with 257 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (4aa38bb) to head (89edf9b).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
state-chain/custom-rpc/src/lib.rs 0% 63 Missing ⚠️
state-chain/node/src/service.rs 0% 62 Missing ⚠️
...ne/src/state_chain_observer/client/base_rpc_api.rs 0% 43 Missing ⚠️
api/lib/src/queries.rs 0% 15 Missing ⚠️
state-chain/runtime/src/monitoring_apis.rs 0% 0 Missing and 14 partials ⚠️
state-chain/chains/src/dot/benchmarking.rs 0% 10 Missing ⚠️
...hunked_chain_source/chunked_by_vault/continuous.rs 0% 8 Missing ⚠️
state-chain/runtime/src/lib.rs 42% 0 Missing and 7 partials ⚠️
.../client/extrinsic_api/signed/submission_watcher.rs 0% 5 Missing ⚠️
...te_chain_observer/client/extrinsic_api/unsigned.rs 0% 4 Missing ⚠️
... and 16 more
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5306     +/-   ##
=======================================
+ Coverage     70%     71%     +1%     
=======================================
  Files        489     487      -2     
  Lines      87875   84720   -3155     
  Branches   87875   84720   -3155     
=======================================
- Hits       61938   60159   -1779     
+ Misses     22630   21854    -776     
+ Partials    3307    2707    -600     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

engine/src/settings.rs Outdated Show resolved Hide resolved
engine-runner-bin/tests/command_args.rs Show resolved Hide resolved
state-chain/amm/src/common.rs Outdated Show resolved Hide resolved
state-chain/cf-integration-tests/src/network.rs Outdated Show resolved Hide resolved
state-chain/chains/src/dot.rs Show resolved Hide resolved
state-chain/pallets/cf-validator/src/lib.rs Show resolved Hide resolved
@kylezs
Copy link
Contributor

kylezs commented Oct 7, 2024

We return anyhow::error for Lots of rpc functions. They are currently converted to ErrorOjbectOwned via to_rpc_error
TODO: review if we should update them to return jsonrpsee::RpcResult instead.

I don't think we need to do this. They are separate things, using an anyhow error means things like the broker api could be used in non-rpc contexts too.

syan095 and others added 17 commits October 7, 2024 16:08
Updated Rust compile version to nightly-2024-06-01
Updated some other library to newer version
Cleaned up the std feature references.

WIP does not build due to feature conflict
* Migrated RPC interface to jsonrpsee-0.23.2
Major changes to how subscriptions are streamed.
Major changes to all error types.

* Improved how errors are converted to ErrorObjectOwned
Updated documentations to be fix warning in later versions.

Cannot upgrade to further version because Substrate code triggers
"unreachable_patterns" error from Clippy.
Based on the diff in polkadot-sdk.
This is more in line with previous behaviour.
@dandanlen dandanlen force-pushed the chore/update-polkadot-sdk-1.15.2 branch from e80df01 to 506974f Compare October 7, 2024 16:10
@dandanlen
Copy link
Collaborator

dandanlen commented Oct 7, 2024

I rebased against main to untangle the Zepter merge with all the feature changes. I don't think it was necessary. Similarly running zepter against the polkadot repo, I would rather avoid such a big change. So I updated the chainflip-substrate-1.15.2+2 tag to a commit without the features change and fixed the remaining feature issues here.

Will take a closer look at non-feature/dependency stuff tomorrow.

Looks good so far though 👌

@dandanlen dandanlen requested review from kylezs and removed request for jerryafr, acdibble and j4m1ef0rd October 7, 2024 16:14
@dandanlen
Copy link
Collaborator

Thanks @syan095 , I added some smallish changes and some larger ones that I'll mention below. I'm happy with this now.

The biggest change I made to the dependencies was to remove a lot of feature declarations. I think these were added by Zepter but most of them are unnecessary / erroneous.

Let's discuss tomorrow, then get it merged 🍾

1. subcommand::try-runtime is deprecated

2. There's a way to change pallet const parameters at Runtime through the use of extrinsic `pallet_parameters::set_parameter`.
   TODO: Do we want to use this feature?

No need for now.

3. Runtime V2: new proc. macro for constructing Runtime.
   TODO: Review:
   
   * All pallets have assigned pallet_index.
   * Current pallet indexes are continuous and ungrouped.
   * should we better group the pallets? (leave gaps in indexes)

✅ LGTM we don't want to change any pallet indices.

4. try-runtime-cli: fully deprecated. Use `paritytech/try-runtime-cli` instead

5. Consider to use workspace dependencies to unify dependency versions so they are consistent across all packages.
   TODO: Consider unify dependency versions

Would be nice to have (and might make future upgrades simpler) but not urgent. I thought we already had a Linear item for this but can't find it right now...

6. TODO: Review - can this be removed?
   #[allow(dead_code)]
   fn input_to_output_amount_floor(amount: Amount, tick: Tick) -> Option;

✅ Removed

7. Deprecated function `itertools::unfold`
   Replaced with `itertools::unfold(|t| {...}`

✅ I think you mean iter::from_fn. I don't like how from_fn works (mutating the environment)... but I guess we don't want to use deprecated functions.

8. Runtime `sp_genesis_builder::GenesisBuilder<Block>` now requires fn preset_names()
   TODO: review if we need to use this feature.
   Overhaul in how "Presets" can be queried.

✅ I changed the implementation slightly. We don't need this now, but it might be useful for configuring different starting conditions for localnets.

9. Runtime: System::Config now has extra fields:
   TODO review if () is OK. Review if we want to refactor all mock Runtimes to use v2 macro.
   type SingleBlockMigrations = ();
   type MultiBlockMigrator = ();
   type PreInherents = ();
   type PostInherents = ();
   type PostTransactions = ();

✅ This is fine but also, these can all be defaulted in the mocks and in the runtime itself. See: 06b6192

10. Aura now need SlotDuration defined explicitly.
    Currently set to SLOT_DURATION (1 block = 6000ms)
    TODO: Review

11. new_full: sc_consensus_grandpa::grandpa_peers_set_config::<_, Network> now has 2 additional fields, `metrics` and `peer_store_handle`
  
    * now require additional type: Libp2p or Litep2p

✅ I reshuffled some of the lines in the service.rs file to make it a closer match to the polkadot-sdk version. This is just to make easier to visually compare the file diffs. I also moved the LibP2p/LiteP2p switch in to command.rs to match how it's done in polkadot-sdk.

12. Jsonrpsee is updated to 0.23.2. Major changes on error types


* Error is now `ErrorObjectOwned`, which contains `code`, `message` and `data`

* TODO: review:
  
  * RPC output encoding/serialization
  * Use of error code CALL_EXECUTION_FAILED_CODE
  * cf_subscribe_scheduled_swaps no long return error
  * new_subscription no longer return error.
  * new_subscription is not async,
    ! on-success: uses substrate version of `pipe_from_stream`
    ! on-fail: spawn a new thread to "reject" the sink.
  * subscription error now wrapped by `ErrorObjectOwned`

✅ All reviewed, LGTM

13. We return anyhow::error for Lots of rpc functions. They are currently converted to ErrorOjbectOwned via `to_rpc_error`
    TODO: review if we should update them to return jsonrpsee::RpcResult instead.

✅ I think anyhow is fine for now. I just changed the implementation to use Display instead of Debug since this is how the errors were formatted previously.

14. TODO: `async fn should_update_version_on_bad_proof()` `Subscription::new()` is now private - the specific test case has been commented out for now, this needs to be fixed/replaced/removed.
    Also Mockall is having trouble creating types that don't have "default" impl. Mockall may need to be replaced here.
    The entire file is commented out right now
    (Jamie said he might pick this up)

Ok, we can follow this up.

15. Statechain observer:
    - updated how RPC errors are being matched.

✅ Looks like we were already matching on the ErrorObject anyway.

16. Lots of error conversion in LP APIs, Broker API, ChainCli, Engine. All anyhow::error are converted into ErrorObjectOwned via `to_rpc_error`. TODO discuss return types (I think it would be easier to always return anyhow::result and map_err() into jsonrpsee error, in case jsonrpsee changes its error type again.

✅ LGTM, we can think about ways to make this more dev friendly but for now it's good enough.

17. `subscribe_order_fills` changed - SubscriptionSinks no longer need to be closed manually.

18. Clap changes: "version_short" removed, auto set to "-V"


* #[clap(parse(from_os_str))] -> #[clap(value_parser(clap::value_parser!(PathBuf)))]

19. Runtime v2 now has "GenesisConfig build test", where all pallets' genesisconfig is built using GenesisConfig::default().
    This is causing test fail in cases such as min_fun = redeption_tax = 0, or when
    initial_authorities = []. Asserts are replaced with `frame_support::print` but return a value to ensure this test will pass.
    I updated the "Default" impl in these cases to make tests pass. This is OK assuming GenesisConfig::Defualt is NEVER used in Production.
    TODO: Review if this is OK.

✅ LGTM

20. Note: Builds with `runtime-benchmarking` cannot run. For building local net, the benchmkaring feature must be turned off.

Seems to work for me - might have been feature-related?

21. TODO: In the next rust version(2024-08-14), "unreachable-pattern" will trigger rust compiler warning. We should review all cases of "unreachable-pattern" (with the unreachable!() macro) and review them

I removed the unreachable pattern exceptions. We almost always want to be alerted on unreachable patterns. What this lint means is that we have written a match statement where one of the arms can never be reached, which is usually a programmer error. It has nothing to do with unrachable!().

22. Getting `lint `clippy::thread_local_initializer_can_be_made_const`has been renamed to`clippy::missing_const_for_thread_local` clippy warning. We don't use this, not sure why we are getting this warning for all our build packages

✅ I found it :) It was in the root Cargo.toml, but named with -s instead of _s. Renamed it and the warnings are gone.

@dandanlen dandanlen force-pushed the chore/update-polkadot-sdk-1.15.2 branch from 25c7dce to 89edf9b Compare October 8, 2024 14:49
@dandanlen dandanlen added this pull request to the merge queue Oct 9, 2024
Merged via the queue into main with commit ab7104b Oct 9, 2024
49 checks passed
@dandanlen dandanlen deleted the chore/update-polkadot-sdk-1.15.2 branch October 9, 2024 12:00
syan095 added a commit that referenced this pull request Oct 9, 2024
…lana-ccm

* origin/main:
  chore: update polkadot sdk 1.15.2, rust to 2024-07-31 (#5306)
  chore: add sol min_prio_fee to ingress and egress estimation (#5323)
  docs: fix log filtering in readme (#5319)
  feat: cleanup aborted broadcasts (#5301)
  fix: increase init timeout for backup test (#5317)
  feat: minimum chunk size setting (#5314)
  chore: add egress fee for token account rent (#5315)
  fix: ensure unused sol deposit channels are closed (#5312)
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.

3 participants