-
Notifications
You must be signed in to change notification settings - Fork 258
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
subxt-core
crate
#1466
subxt-core
crate
#1466
Conversation
core/Cargo.toml
Outdated
[features] | ||
default = ["std"] # "std" | ||
std = [ | ||
"scale-info/std", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are not all crates that has default-features = false
added here?
For instance codec/std is not enabled and many more....
subxt/src/client/offline_client.rs
Outdated
@@ -21,6 +21,8 @@ pub trait OfflineClientT<T: Config>: Clone + Send + Sync + 'static { | |||
fn genesis_hash(&self) -> T::Hash; | |||
/// Return the provided [`RuntimeVersion`]. | |||
fn runtime_version(&self) -> RuntimeVersion; | |||
/// Return the [subxt_core::client::ClientState] (metadata, runtime version and genesis hash). | |||
fn client_state(&self) -> ClientState<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have this method, then let's give it a default impl too since it's just a concat of a few of the above :)
|
||
/// Return the [subxt_core::client::ClientState] (metadata, runtime version and genesis hash). | ||
pub fn client_state(&self) -> ClientState<T> { | ||
let inner = self.inner.read().expect("shouldn't be poisoned"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess even with a default impl we'll want this bit anyay though to avoid multiple locks, but it could be seen as an optimisation anyway, so it's all good :)
This is great stuff; left another round of comments but all pretty minor things, and we can thing more about docs etc later :) @tadeohepperle if you have time to address the small comments and get things compiling then I'm happy to see this merge now! |
@@ -31,22 +33,15 @@ pub trait ConstantAddress { | |||
#[derivative( | |||
Clone(bound = ""), | |||
Debug(bound = ""), | |||
Eq(bound = ""), | |||
Ord(bound = ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no Ord impl anymore?
/// A static address to a custom value. | ||
#[derive(Derivative)] | ||
#[derivative( | ||
Clone(bound = ""), | ||
Debug(bound = ""), | ||
Eq(bound = ""), | ||
Ord(bound = ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here no Ord?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is these changes related to this refactoring? Lots of changes at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there should be a lot of changes, because we use a lot of types from subxt_core
now instead of pulling them from subxt
. Additionally we use rust prelude types from alloc
instead of from std
.
testing/no-std-tests/Cargo.toml
Outdated
subxt-signer = { path = "../../signer", default-features = false, features = ["sr25519"] } | ||
subxt-core = { path = "../../core", default-features = false } | ||
subxt-signer = { path = "../../signer", default-features = false, features = ["subxt", "sr25519"] } | ||
subxt-macro = { path = "../../macro" } # Not used yet, but will be used here very soon, to verify the generated code is applicable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty slow and don't understand the comment!
I suppose you mean that we don't test that the generated code is no-std?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this should be removed, we now test that the generated code compiles in no-std
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, great job.
just some small comments
We need another fix in |
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/parity-tech-update-for-march/7226/1 |
Introduces a
subxt-core
crate that provides ano_std
compatible subset ofsubxt
's functionality.