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

Make subxt-core ready for publishing #1508

Merged
merged 27 commits into from
Apr 15, 2024
Merged

Make subxt-core ready for publishing #1508

merged 27 commits into from
Apr 15, 2024

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Mar 28, 2024

Tadeo did some great work getting a bunch of no-std compatible bits moved out of subxt into this new subxt-core crate. The aim of this PR is to tidy up and polish this crate so that it's ready to be released and made use of directly.

Some things I did here:

  • Move Extrinsic decoding stuff to subxt_core
  • Move Transaction creation/signing stuff to subxt_core
  • Expose methods in storage and runtime APIs to encode/decode things and use in Subxt
  • Provide a top level example for each of the areas (tx, runtime_apis, events, constants, custom_values, storage, blocks) to show how to build/encode/decode the relevant bits, and overall docs to link to these main areas.
  • Various other tweaks to consistify the structure and such.

@jsdw jsdw changed the title Jsdw subxt core polish Polish subxt-core Mar 28, 2024
Comment on lines +157 to +160
let runtime_version = subxt::client::RuntimeVersion {
spec_version: 9370,
transaction_version: 20,
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted to just a struct so that the args (of the same type) were named

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more specific, I like it

Comment on lines +232 to +236
if absolute_path(crate_path.clone()).is_err() {
// Throw an error here, because otherwise we end up with a harder to comprehend error when
// substitute types don't begin with an absolute path.
panic!("The provided crate path must be an absolute path, ie prefixed with '::' or 'crate'");
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We shouldn't need to clone crate_path here but need to change scale-typegen to avoid.

@@ -10,7 +12,7 @@ use derive_where::derive_where;

/// This represents a constant address. Anything implementing this trait
/// can be used to fetch constants.
pub trait ConstantAddress {
pub trait AddressT {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I standardised on all addresses/payloads being either AddressT (just looking something up; constants & storage) or PayloadT (submitting/calling something; TX's & runtime APIs)

Copy link
Member

Choose a reason for hiding this comment

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

perhaps modify the docs then above 👆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you think needs modifying? I think they are still relevant? :)

Copy link
Member

Choose a reason for hiding this comment

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

yepp, I misunderstood your comment

///
/// When the provided `address` is dynamic (and thus does not come with any expectation of the
/// shape of the constant value), this just returns `Ok(())`
pub fn validate<Address: AddressT>(metadata: &Metadata, address: &Address) -> Result<(), Error> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

instead of validate_constant etc, aim for constants::validate etc.

}

/// Some extension methods on [`subxt_metadata::Metadata`] that return Errors instead of Options.
pub trait MetadataExt {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't see a need for this offhand and just added the one missing function above on the concrete type instead.

Copy link
Collaborator Author

@jsdw jsdw Apr 9, 2024

Choose a reason for hiding this comment

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

This was just moved from mod.rs; I should have done a git mv because after then changing mod.rs it didn't pick up the move.

Comment on lines -29 to -46
/// Outputs the [`storage_address_root_bytes`] as well as any additional bytes that represent
/// a lookup in a storage map at that location.
pub fn storage_address_bytes<Address: StorageAddress>(
addr: &Address,
metadata: &Metadata,
) -> Result<Vec<u8>, Error> {
let mut bytes = Vec::new();
write_storage_address_root_bytes(addr, &mut bytes);
addr.append_entry_bytes(metadata, &mut bytes)?;
Ok(bytes)
}

/// Outputs a vector containing the bytes written by [`write_storage_address_root_bytes`].
pub fn storage_address_root_bytes<Address: StorageAddress>(addr: &Address) -> Vec<u8> {
let mut bytes = Vec::new();
write_storage_address_root_bytes(addr, &mut bytes);
bytes
}
Copy link
Collaborator Author

@jsdw jsdw Apr 9, 2024

Choose a reason for hiding this comment

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

moved to mod.rs as get_address_bytes and get_address_root_bytes

@jsdw jsdw marked this pull request as ready for review April 9, 2024 16:16
@jsdw jsdw requested a review from a team as a code owner April 9, 2024 16:16
@jsdw jsdw changed the title Polish subxt-core Make subxt-core ready for publishing Apr 9, 2024
pub fn genesis_hash(&self) -> C::Hash {
self.genesis_hash
}
/// Genesis hash.
Copy link
Member

Choose a reason for hiding this comment

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

why pub now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think is more in-line with RuntimeVersion, but either way is good for me

Copy link
Member

Choose a reason for hiding this comment

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

It's probably because we use it outside subxt-core but ideally we wouldn't make it pub. It's hard to add stuff without breaking the API...

Copy link
Member

Choose a reason for hiding this comment

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

I would probably add doc(hidden) to it to be on the safe-side

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I made runtime version things pub just because it felt better constructing it with named params (and it's basically just a named tuple). I think I followed the same logic here and just made it pub since it's just a tuple of 3 things you can fetch individually otherwise. I could go either way really, but at the time preferred the simplicity I guess of it just being a simple data struct :)

I don't think it should be doc(hidden) personally because it's a used as a param in a couple of places so people should be able to name it.

@@ -1,4 +1,4 @@
// Copyright 2019-2023 Parity Technologies (UK) Ltd.
// Copyright 2019-2024 Parity Technologies (UK) Ltd.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should we rename Singer -> SignerT for consistency with AddrT / PayloadT?
I've also seen it imported as SignerT for testing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! There are probably a bunch of other traits here that we could, if we wanted, add T on the end of! In this PR I just standardised the "address/payload" traits to have a T but maybe we should go further! What do you reckon? (miight be better to do a full standarisation as a separate PR but I'd have to check hoiw many traits we have left to add the T too :D)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a look at the other traits we have and now I'm not sure which way is best! I liked the standardising here but also think that adding T onto everything feels a bit naff (and it's not what Rust does on any of the basic traits).

Lemme have a play; maybe I should undo the T thing and do something else..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I'll sort this out in a small separate PR anyways :))

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nice one 🚀

@jsdw jsdw merged commit 1e111ea into master Apr 15, 2024
13 checks passed
@jsdw jsdw deleted the jsdw-subxt-core-polish branch April 15, 2024 14:20
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/paritytech-update-for-april/7646/1

@jsdw jsdw mentioned this pull request May 16, 2024
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.

None yet

4 participants