-
Notifications
You must be signed in to change notification settings - Fork 784
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
Maintain transaction lifetime on transactions #2484
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @mcintyre94 and the rest of your teammates on Graphite |
44fa190
to
68d0c5e
Compare
export async function partiallySignTransactionMessageWithSigners< | ||
TTransactionMessage extends CompilableTransactionMessageWithSigners = CompilableTransactionMessageWithSigners, | ||
>(transactionMessage: TTransactionMessage, config: { abortSignal?: AbortSignal } = {}): Promise<NewTransaction> { | ||
>( | ||
transactionMessage: TTransactionMessage, | ||
config: { abortSignal?: AbortSignal } = {}, | ||
): Promise<NewTransaction & TransactionWithLifetime> { |
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.
Should we have a helper type like GetTransactionLifetimeFromTransactionMessage<TTransactionMessage>
?
Then here we can return Promise<NewTransaction & GetTransactionLifetimeFromTransactionMessage<TTransactionMessage>>
which means TypeScript doesn't loose that information?
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 tried to do this (playground) but it doesn't seem to want to be that clever
I've added type overloads for the lifetime types instead to mirror what we do with compileTransactionMessage
, and updated the typetests
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.
Oh weird. I thought that's what we were doing with TClusterUrl
on RPC packages but there must be some subtle change there that makes TS happy. 🤷♂️
68d0c5e
to
4c0aed8
Compare
4c0aed8
to
e24a32b
Compare
🎉 This PR is included in version 1.91.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
This PR updates the usages of
NewTransaction
to maintain the transaction lifetime. I should have realised we'd have to extend it!The sign functions in the
transactions
package now take aT extends NewTransaction
and maintain the type T. This means that if you input a transaction with a lifetime, the output transaction will have the same lifetime.The signers now return
NewTransaction & TransactionWithLifetime
. I haven't calculated the diff here because they take a transaction message as input and callcompileTransactionMessage
, so their output isn't based on the transaction message input.