Rework Subxt API to support offline and dynamic transactions #593
Conversation
| .metadata() | ||
| .constant_hash(address.pallet_name(), address.constant_name())?; | ||
| if actual_hash != expected_hash { | ||
| return Err(MetadataError::IncompatibleMetadata.into()) |
There was a problem hiding this comment.
When this fails it would be super useful if the actual pallet constant name is embedded in the error.
This is hard to debug otherwise.
There was a problem hiding this comment.
Yeahh Andrew raised this too; it's a separate issue somewhere so I didn't want to think about it in this PR!
| @@ -273,9 +294,10 @@ impl PalletMetadata { | |||
| /// Metadata for specific events. | |||
| #[derive(Clone, Debug)] | |||
| pub struct EventMetadata { | |||
| pallet: String, | |||
| pallet: Arc<str>, | |||
There was a problem hiding this comment.
can you explain why this is changed to an Arc?
There was a problem hiding this comment.
Ah sure; Basically because the pallet name is duplicated across every event in a pallet ,so instead of clone()int a String to store with every event I just stuck it behind an arc so that it's an increment instead.
Probably worth a comment in the code :)
|
Ah, the checks ran foul of some recent substrate build change that we picked up on a couple of nights ago; guess I'll have to fix it now! |
subxt/src/storage/utils.rs
Outdated
|
|
||
| /// Return the root of a given [`StorageAddress`]: hash the pallet name and entry name | ||
| /// and append those bytes to the output. | ||
| pub fn storage_address_root_bytes<Address: StorageAddress>( |
There was a problem hiding this comment.
I think the naming of this function is a bit confusing.
I would prefer some like write_storage_address_root or something like that.
There was a problem hiding this comment.
Good catch; these names were rubbish! I changed them both :)
| &self, | ||
| call: &Call, | ||
| signer: &(dyn Signer<T> + Send + Sync), | ||
| other_params: <T::ExtrinsicParams as ExtrinsicParams<T::Index, T::Hash>>::OtherParams, |
There was a problem hiding this comment.
Should we borrow other_params too? Consistent with other params and doesn't require Clone for copying instances.
There was a problem hiding this comment.
I had a go at this, and where I landed was:
- If we can accept a reference to
OtherParamswhen buildingExtrinsicParamsthat'd be nice, and probably reflect how it's used the best (and then passing references in would be ideal). But it turns out to be a pain to do actually, so the complexity didn't feel worth while. - If we can't, then we need to do a
.clone()call internally, and I'd rather push that to the user (and then impls can be copy potentially and it's no biggie, or they can't and better the user has the chance to pass ownership rather than force a clone.
So for now I'd suggest leaving it as it is, but we might be able to revisit and improve!
| /// Events decoding error. | ||
| #[error("Events decoding error: {0}")] | ||
| EventsDecoding(#[from] DecodeError), | ||
| Runtime(DispatchError), |
subxt/src/tx/tx_progress.rs
Outdated
| tracing::warn!("Can't decode error: sp_runtime::DispatchError::Module details do not match known information"); | ||
| return DispatchError::Other(bytes.to_vec()) | ||
| } | ||
| }; |
| .unwrap(); | ||
|
|
||
| api.rpc() | ||
| .dry_run(signed_extrinsic.encoded(), None) |
subxt/src/tx/tx_progress.rs
Outdated
| // Attempt to decode a DispatchError, returning either the ModuleError it decodes to, | ||
| // along with additional details on the error, or returning the raw bytes to work with | ||
| // downstream if we couldn't decode it. | ||
| fn decode_dispatch_error(metadata: &Metadata, bytes: &[u8]) -> DispatchError { |
There was a problem hiding this comment.
I may need access to this, see: https://github.com/paritytech/cargo-contract/blob/master/src/cmd/extrinsics/mod.rs#L246.
Essentially I want to decode the DispatchError returned from an RPC call rather than this extrinsic.
Could we hang it off Metadata? Or DispatchError?
There was a problem hiding this comment.
Ooh yup, sounds very reasonable to me! I stuck it onto DispatchError
(this might land in this PR in an early stage to be built on further).Tips for Reviewers
The best place to start is probably by looking at the examples and tests to see what using the new APIs looks like. Inparticular, a new
dynamic_queries.rsexample mirrors some of the other examples but shows how you'd use purely runtime Values and no codegen to do things.Next, the
clientfolder is the API entry point, and from there you can look at the individual TxClient, StorageClient, ConstantsClient and EventClient to see what methods are available for each and what data is accepted.These clients generally accept Address/Payload traits, for which we have an implementation used by the static codegen and another used for submitting dynamic Values. This means that the actual API is tied to neither approach.
From here, the constants, events. storage and tx folders contain the respective logic for each area of the API.
storage/storage_address.rs,ts/tx_payload.rs,constants/constant_address.rshave the traits and static/dynamic implementations for submitting things to the API.A
dynamic.rsfile/module exists as a shortcut to all things dynamic (dynamic::tx(),dynamic::storage()) etc. These are just aliases to the relevantdynamicfunction in each location, so that it's easy to find everything in one place.This closes #581, closes #406, closes #592 and closes #582