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

chore: improve the extensibility of (Signing)StargateClient #1099

Closed
wants to merge 3 commits into from
Closed

chore: improve the extensibility of (Signing)StargateClient #1099

wants to merge 3 commits into from

Conversation

RiccardoM
Copy link
Contributor

This PR improves the overall code of StargateClient and SigningStargateClient. The main changes are the following:

  • introduced a new StargateClientOptions interface that allows setting custom options for StargateClient.
    The most important option is the accountParser option, which must be an AccountParser instance and allows to provide the StargateClient a custom way of decoding Cosmos accounts. This is particularly useful to solve problems when reading account from chains that support custom account types (eg. the Desmos chain). This can solve problems like the one REStake is having: Desmos Failed to broadcast: Unsupported type eco-stake/restake#334
  • changed the return type of the signDirect, signAmino and sign methods to be SignatureResult instead of TxRaw.
    This allows clients to also get the data that has been used when generating the signature. This is particularly useful in cases where the resulting signature must later be verified offline (eg. in a signature-based authentication process). Without the data (account number, sequence and sign doc) used, the client would have to re-generate the signed bytes by hand. With this change, instead, the sign bytes are returned by the sign method directly without any other operations needed.
  • made the signer, aminoTypes and registry fields protected instead of private.
    This allows classes extending the SigningStargateClient to easily access those fields if needed (eg. when performing operations that need to encode a message or else)
  • added the SigningMode interface and the getSigningMode method to StargateSigningClient.
    These allow classes extending SigningStargateClient to easily implement new ways of defining what signing algorithm should be used. For example, developers might want to define a custom way of choosing which signing algorithm to be used that is based on what a signer supports. We could have a signer that uses WalletConnect with multiple versions on the market: a way to determine if the direct method is supported might be to ask directly the signer rather than performing type checks

@RiccardoM
Copy link
Contributor Author

@webmaster128 Is it possible to have a review to this? I think it might be quite helpful to developers building on top of CosmJS

@webmaster128
Copy link
Member

Thank you, @RiccardoM! Lots of good stuff, lots of breaking changes.

Before we go into detail, there are two big movements regarding extensibility:

  1. Configure the StargateClient
  2. Make it easy to create custom clients based on a custom selection of modules

Now 1. is pushing for more and more flexibility while 2. is pushing for a more lightwight client that serves as an example for custom clients once you do something fancy.

This PR uses approach 1., but in two different ways:
(a) Configuration via options
(b) Extensibility

This feels inconsistent. E.g. why don't we use overriding for the accountParser? Do we want to require users to create their own client?

Also I have a different question: I recently rejected the request for a custom auth query in #1082. Is this a problem Desmos also has? Or is it "just" custom account responses?

Overall, happy to bring those changes in but we need to break it down.

@RiccardoM
Copy link
Contributor Author

This feels inconsistent. E.g. why don't we use overriding for the accountParser? Do we want to require users to create their own client?

I think that using the option might be a lot better for developers that want to use the standard StargateClient implementation but also support custom accounts queries. I believe that developers should be forced to extend the default StargateClient only when they want to implement custom behaviors (maybe custom gRPC queries or other operations).

Ideally, even the latter (extend the functions) could be implemented in a way that's easier for developers and do not require them to create a custom Client class (maybe using the same way that it's now being used for QueryClient or something similar). But anyway, this is for another issue.

Also I have a different question: I recently rejected the request for a custom auth query in #1082. Is this a problem Desmos also has? Or is it "just" custom account responses?

Desmos does not have that issue. I think the most common issue we will see among clients that interact with different Cosmos chain is going to be about parsing custom account types. That's why this PR focuses on that only.

@webmaster128
Copy link
Member

Cool 👍 Can you extract the AccountParser change in a separate PR? This is a non-breaking change that is relatively straight forward and can be shipped in 0.28.x.

@RiccardoM
Copy link
Contributor Author

Cool +1 Can you extract the AccountParser change in a separate PR? This is a non-breaking change that is relatively straight forward and can be shipped in 0.28.x.

Done inside #1102

*/
protected getSigningMode(): SigningMode {
return isOfflineDirectSigner(this.signer) ? SigningMode.Direct : SigningMode.Amino;
}
Copy link
Member

Choose a reason for hiding this comment

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

This problem came up with Keplr before. The type itself is insufficient to determine the sign mode side it could be either a software wallet or a Ledger behind Keplr.

Woudn't it make sense to extend the OfflineSigner in a way that the sign mode can be determined at runtime instead of compine time?

Would this avoid that clients have to change the logic of this function?

authInfoBytes: signedAuthInfoBytes,
signatures: [fromBase64(signature.signature)],
}),
};
Copy link
Member

Choose a reason for hiding this comment

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

Can't you solve the same problem in the app by manually querying account number and sequence an passing it as explicitSignerData?

The change here is not wrong, but creates additional complexity for the average use case and is breaking.

@webmaster128
Copy link
Member

Closing as stale. As discussed before, let's split those changes into small chunks such that we can better separate in breaking and non-breaking changes. Happy for such additions in general.

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.

2 participants