-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
[ts-sdk] Define the Provider Interface #1331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change the way this is setup to eliminate the need for null/undefined checking in the signer (by making the constructor set the values).
sdk/typescript/src/signers/signer.ts
Outdated
async transfer( | ||
transaction: TransferTransaction | ||
): Promise<TransactionResponse> { | ||
this._checkProviderAndSerializer('transfer'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like we're always checking for prerequisite objects before doing anything, but i'm not sure we gain any significant flexibility by allowing these objects to be null/undefined at all.
Can we set the provider & serializer in the constructor (they're already readonly)?
that way they're always defined.
If users want to change the serializer or provider, they can construct a new signer class instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some use cases might just need a signer without a provider, e.g., to produce pre-signed message, to prove that the user owns this address, etc. This latter use case is very popular in some portfolio tracking applications such as https://debank.com/.
My demo app also uses similar pattern: https://gist.github.com/666lcz/6cc6c983e841eda9f7a457e47bc473b7 without a provider.
Ether.js is also using a similar pattern: https://github.com/ethers-io/ethers.js/blob/948f77050dae884fe88932fd88af75560aac9d78/packages/abstract-signer/src.ts/index.ts#L98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, now that i think about this some more - I think it seems like we should completely remove the provider from the signer. Signing & sending the tx seem like two separate concerns.
I think the ideal signer might be something that implements only
interface Signer {
signData(data: Base64DataBuffer): Promise<SignaturePubkeyPair>
}
Codecov Report
@@ Coverage Diff @@
## main #1331 +/- ##
==========================================
+ Coverage 81.79% 82.41% +0.62%
==========================================
Files 100 103 +3
Lines 20972 20801 -171
==========================================
- Hits 17153 17143 -10
+ Misses 3819 3658 -161
Continue to review full report at Codecov.
|
Addressed @stella3d 's comment by:
With these changes, we keep the The code snippets below will demonstrate why having a With the
If we always have the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This latest update looks good, it addresses my previous concerns with initialization of dependency objects. great work
For more context, please see https://github.com/MystenLabs/sui/pull/1308/files.
This PR defines the barebone interface for
Provider
. Subsequent PRs will add more methods and implement the method using a JSON-RPC client