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

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

Closed
6 of 18 tasks
ascjones opened this issue Nov 3, 2021 · 3 comments
Closed
6 of 18 tasks

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

ascjones opened this issue Nov 3, 2021 · 3 comments

Comments

@ascjones
Copy link
Contributor

ascjones commented Nov 3, 2021

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:

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). Static metadata validation #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. Refactor duplication for generation of composite fields #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
Copy link
Contributor

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
Copy link
Contributor Author

ascjones commented Nov 4, 2021

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
Copy link
Collaborator

jsdw commented May 24, 2022

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

@jsdw jsdw closed this as completed May 24, 2022
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

No branches or pull requests

3 participants