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

Update substrate #370

Merged
merged 20 commits into from
Jul 14, 2023
Merged

Update substrate #370

merged 20 commits into from
Jul 14, 2023

Conversation

JesseAbram
Copy link
Member

@JesseAbram JesseAbram commented Jul 11, 2023

updates substrate, subxt as well as others

Notes

AccountId32 -> requiers conversions between the two, subxt no longer accepts sp_core, and sp_core is needed in PartyId because it implements hash while subxt does not

@vitropy this changes the devnet launch scripts slightly, no rush just tagging so you are aware (we talked offline about this)

@JesseAbram JesseAbram marked this pull request as ready for review July 13, 2023 20:24
@@ -195,6 +195,7 @@ pub fn new_partial(

let (grandpa_block_import, grandpa_link) = grandpa::block_import(
client.clone(),
#[allow(clippy::redundant_clone)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@ameba23 ameba23 left a comment

Choose a reason for hiding this comment

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

Well done for getting through this, it looks like it was an absolute nightmare!

I certainly don't see any red flags, but i really don't understand the weights stuff, which looks like it has changed significantly, nor do i understand the substrate benchmarks. I'd like to take some time to get my head around it, but thats not gonna happen before i am away camping, so i defer to @jakehemmerle

use subxt::{
ext::{
sp_core::{
sr25519::{self, Pair},
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't understand whats going on here. we were getting sr25519, Pair and Bytes from sp_core and now we are getting them from subxt::ext::sp_core. Which looks to me like it is a re-export of the exact same version of sp_core

sp-core ={ package="sp-core", git="https://github.com/paritytech/substrate", branch="polkadot-v0.9.31" }
subxt ={ package="subxt", git="https://github.com/paritytech/subxt.git", tag="v0.29.0" }
parity-scale-codec="3.4.0"
sp-core ={ version="21.0.0", default-features=false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sure there is a good reason - but why does server and kvdb use a newer version of sp-core than entropy, entropy-shared and entropy-constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

good find, tbh they are not used, have removed, I had to change sp-core to match the subxt version to save on a lot......a lot of errors lol

@JesseAbram
Copy link
Member Author

Well done for getting through this, it looks like it was an absolute nightmare!

I certainly don't see any red flags, but i really don't understand the weights stuff, which looks like it has changed significantly, nor do i understand the substrate benchmarks. I'd like to take some time to get my head around it, but thats not gonna happen before i am away camping, so i defer to @jakehemmerle

tbh most of it is auto generate, looks like they added some more precise calculation

@vitropy
Copy link
Contributor

vitropy commented Jul 14, 2023

@vitropy this changes the devnet launch scripts slightly, no rush just tagging so you are aware (we talked offline about this)

I'm not aware of the context of this code enough to immediately understand what changes to the devnet automations are needed. Can you list them out for me or maybe we touch base synchronously soon?

Copy link
Contributor

@jakehemmerle jakehemmerle left a comment

Choose a reason for hiding this comment

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

Nice PR, nothing really that I'd change.

Regarding the AccountId32 stuff, you might have seen this subxt CHANGELOG entry explaining is the subxt::utils::AccountId32 type. After further looking, it impls From<sp_core::crypto::AccountId32> and From<sp_runtime::AccountId32> (when using the substrate-compat feature which is enabled by default)

If that's not new information, then this is good to go. If it is, then there might be places we're using subxt::utils::AccountId32 where we could just be using sp_runtime::AccountId32 with an .into() or something.

crypto/testing-utils/src/chain_api.rs Outdated Show resolved Hide resolved
@JesseAbram
Copy link
Member Author

Nice PR, nothing really that I'd change.

Regarding the AccountId32 stuff, you might have seen this subxt CHANGELOG entry explaining is the subxt::utils::AccountId32 type. After further looking, it impls From<sp_core::crypto::AccountId32> and From<sp_runtime::AccountId32> (when using the substrate-compat feature which is enabled by default)

If that's not new information, then this is good to go. If it is, then there might be places we're using subxt::utils::AccountId32 where we could just be using sp_runtime::AccountId32 with an .into() or something.

ya, this PR is big enough as it is and there are things I wanna get done before our retreat, imma go with let's merge it and then put up a refactor card and come back to it with fresh eyes

@JesseAbram JesseAbram merged commit dff3249 into entropyxyz:master Jul 14, 2023
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.

4 participants