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

Don't require Pair and ExtrinsicParams trait bound for getter implementations. #357

Closed
clangenb opened this issue Dec 6, 2022 · 11 comments
Assignees
Labels
F7-enhancement Enhances an already existing functionality

Comments

@clangenb
Copy link
Collaborator

clangenb commented Dec 6, 2022

We always require Pair or ExtrinsicsParams for all the Api implementations. This leads to the weird situation, where an actual pair needs to be supplied even if we never set a signer, as we don't need to sign anything, e.g.:

let api = Api::<sr25519::Pair, _, AssetTipExtrinsicParams>::new(client).unwrap();

If we remove the trait bound for all getters, we can instantiate the Api like this, which is much simpler:

let api =  Api::<(), _, ()>::new(client).unwrap()
@clangenb clangenb changed the title Don't require Pair amd ExtrinsicParams trait bound for getter implementations. Don't require Pair and ExtrinsicParams trait bound for getter implementations. Dec 6, 2022
@clangenb clangenb added the F7-enhancement Enhances an already existing functionality label Dec 6, 2022
@haerdib

This comment was marked as outdated.

@haerdib haerdib self-assigned this Dec 7, 2022
@haerdib

This comment was marked as resolved.

@clangenb

This comment was marked as outdated.

@haerdib

This comment was marked as outdated.

@haerdib
Copy link
Contributor

haerdib commented Dec 7, 2022

The Pair problem should be solved with PR #356. The ExtrinsicParams will need to be solved in a separate PR

@haerdib haerdib mentioned this issue Dec 9, 2022
@haerdib haerdib removed their assignment Dec 9, 2022
@haerdib haerdib self-assigned this Jan 4, 2023
@haerdib
Copy link
Contributor

haerdib commented Jan 4, 2023

There's one problem with ExtrinsicParams: extrinsic_params_builder needs to be of type Params::OtherParams:

extrinsic_params_builder: Option<Params::OtherParams>,

So simply moving the function to a different impl will not solve this.

I see the following options:

  1. Empty impl of ExtrinsicParams. See Do not require extrinsic params #410
  2. Do not return ExtrinsicParams. Instead let the Api implement the trait to return additional_signed and signed_extra:
    /// These are the parameters which are sent along with the transaction,
    /// as well as taken into account when signing the transaction.
    fn signed_extra(&self) -> Self::SignedExtra;
    /// These parameters are not sent along with the transaction, but are
    /// taken into account when signing it, meaning the client and node must agree
    /// on their values.
    fn additional_signed(&self) -> Self::AdditionalSigned;

    Drawback: There will be no "independent" extrinsic parameter struct anymore.
  3. Restructure the Api: Let not the api hold the values of runtime_version, metadata and such, but instead the ExtrinsicParams themselves. This way, the ExtrinsicParams value would need to be set, not the Params::AdditionalParams, allowing to remove the trait enforcement.

What's your take on that @clangenb ?

@clangenb
Copy link
Collaborator Author

I don't like 1. and 2., but 3. is an option for me, but it goes against the initial goal to be able to have () in the api-client as we still want to have runtime-version and metadata.

Maybe just separate the api into:

GetterClient
ExtrinsicSender
FullApiClient

Or lastly, perhaps we should just combine the all those fields into Config struct, where we can also supply one that works for default chains usually similar to subxt.

@haerdib
Copy link
Contributor

haerdib commented Jan 10, 2023

If we separate, then I believe two are enough. The ExtrinsicSender needs to be able to get things as well. At least to know its nonce. Therefore, I'd vote for GetterClient and FullApiClient. Three would just complicate the matter.

Hm.. maybe yes to the Config struct. But maybe both make sense, as the GetterClient would be a lot simpler than the FullApiClient, not needing any node information at all.

If it's alright with you, I'll create a follow-up issue with a GetterClient creation and close this one. The Config issue already exists.

@clangenb
Copy link
Collaborator Author

This is ok for me, however, my ideas were just a brainstorm, take it with a grain of salt.

@haerdib
Copy link
Contributor

haerdib commented Jan 10, 2023

Don't worry. I was thinking about that as well. And I think subxt has a similar concept with offline and online client.

@haerdib
Copy link
Contributor

haerdib commented Jan 10, 2023

Closing this issue in favour of #432 and #406

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F7-enhancement Enhances an already existing functionality
Projects
None yet
Development

No branches or pull requests

2 participants