paritytech/subxt

Follow ups to https://github.com/paritytech/subxt/pull/294

ascjones opened this issue · 3 comments

ascjones commented

As discussed with @dvdplm @jsdw and @insipx last week, we should aim to merge #294 as soon as possible in an incomplete state, then continue work on it on the master branch with smaller PRs to complete the remaining todos.

Here is an unordered and non-exhaustive list of the code todos (these can be promoted and fleshed out in separate issues as required). We need to further triage these to decide what we require before making a release.

todos extracted from code:

  • Maintain any regular Rust code items from the decorated mod. At the moment they will be erased. Possibly by implementing ToTokens on ItemMod. 6306b9f #346
  • Allow user to override default Config runtime types as part of mod annotated with subxt macro.

    subxt/codegen/src/api/mod.rs

    Line 227 in 5dd6cf9

    pub struct DefaultConfig;
    #347
  • Allow user to define default Extra implementation for additional signed data, or auto-generate from the metadata. At the moment this is hardcoded in the macro generated code.

    subxt/codegen/src/api/mod.rs

    Line 245 in fc30e90

    type Extra = ::subxt::DefaultExtra<DefaultConfig>;
    #331
  • Currently the AccountData storage type is hardcoded, which is required for fetching the nonce for constructing the Extra data for an Extrinsic. It assumes the presence of the system pallet and that the AccountData type has a nonce field. So we should consider whether this should be made configurable, or at least detect if the system pallet is not present with that Storage entry. Currently we are assuming it is always there (note: also true of DispatchError).

    subxt/codegen/src/api/mod.rs

    Line 248 in 63bb5ea

    pub type AccountData = self::system::storage::Account;
    #348
  • Codegen type substitution tests and examples. 6886f4c
  • Remove hack to detect fields with Box type

    subxt/codegen/src/types/type_def.rs

    Line 203 in 63641b4

    if ty_name.contains("Box<") {
    . Depends on paritytech/scale-info#82
  • Revisit handling of compact type codegen, does it work in all cases, can it be improved?
  • Sudo pallet, can/should we handle calls in a non-strongly typed way, i.e allow opaque encoded calls? Strongly typed calls could fail if static metadata used to generated enums is different from runtime metadata of the node. 06a6ddf

PR feedback

Extracted to #419

Misc

  • [converted to issue] Add validation to check if static metadata used to generate API differs from the metadata fetched from the running node at runtime. (suggested by @dvdplm). We could configure how permissive to be here e.g. allow txs as long as the structure of the specific call is the same, but the runtime is a newer version (possibly because of another pallet having been updated). #398

Documentation and Testing

  • Main README needs updating
  • More module level code docs
  • All public types and functions to be documented
  • Add more examples and tests for other FRAME pallets
  • Add more unit tests e.g. for codegen
  • Add documentation about distinction between usage of metadata at compile time to generate the API, and metadata at runtime used to construct extrinsic indices and decode events

Refactoring and code cleanup

  • Duplication of code between codegen/types/type_def and codegen/struct_def for generating composite types. #328
  • More general refactoring and tidy up of codegen module.
  • Role of Client now? I removed lots of pass through rpc methods there, maybe it can be removed/refactored away.
  • There are some types copied from substrate lying around e.g. Phase and WrapperTypeOpaque. This is to avoid depending on big heavy crates just to use one or two types that are fairly static. These could be grouped together in their own files, so it is clear they have been copied. Further, we could possibly even test whether they are still compatible with the original types by using the scale-info metadata fetched from a running test node.

API improvements.

API construction

Currently relies on using some types generated by the macro to construct the runtime API, which is not very user friendly. How can we improve this?

 let api = ClientBuilder::new()
        .build()
        .await?
        .to_runtime_api::<polkadot::RuntimeApi<polkadot::DefaultConfig>>(); // <-- using RuntimeApi and DefaultConfig from macro generated code
seunlanlege commented

Fire & Forget vs Event Subscription API

With the current api it's hard to get transaction finality notifications. submit_and_watch should return a Stream<Item=TransactionStatus>

enum TransactionStatus {
    InPool
    InBlock(Hash)
    Finalized
    Retracted
}
ascjones commented

Yes indeed, @seunlanlege, something like that would be good. If you have time, could you open an issue for it to flesh out and discuss a possible API.

jsdw commented

Given how much has changed since this, I'll close this for now. If anything in here matters to anybody, please open separate issues for each concern and we'll address them there

  • 👍1