-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat: separate sign and send #987
feat: separate sign and send #987
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.
Looks pretty good! My biggest gripe is that SentTransaction.init
now takes assembled
, which is an AssembledTransaction
, and signed
, which is already an attribute on assembled
. I'd rather it just take AssembledTransaction
, or it takes the signed
plus the raw attributes that it needs from AssembledTransaction
. I think that would be:
rpcUrl
allowHttp
timeoutInSeconds
parseResultXdr
Not too bad. Maybe worth getting rid of the assembled
argument?
better user instructions Co-authored-by: Chad Ostrowski <[email protected]>
Sounds good, I will try removing the assembled argument and just pass the necessary attributes from AssembledTransaction. |
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 seems like purely a refactor, right? No real functionality/code changes besides the +2 new methods? LGTM!
@@ -53,61 +49,38 @@ export class SentTransaction<T> { | |||
}; | |||
|
|||
constructor( | |||
public signTransaction: ClientOptions["signTransaction"], | |||
public assembled: AssembledTransaction<T>, | |||
public options: SentTransactionOptions<T>, |
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 wait this is a breaking change, right? ⛔
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.
Yeah, good call, we need to add that to the breaking changes
section of the changelog
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.
Is there any way we can avoid it? It's too late for breaking changes since we already tagged off a non-RC build.
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.
Sure, let me revert that change, it's going to make for some silly code but that's better than a breaking change
* @typedef {Object} SentTransactionOptions | ||
* @property {number} [timeoutInSeconds] - Optional timeout duration in seconds for the transaction. | ||
* @property {string} rpcUrl - The RPC URL of the network to which the transaction will be sent. | ||
* @property {boolean} [allowHttp] - Optional flag to allow HTTP connections (default is false, HTTPS is preferred). | ||
* @property {(xdr: xdr.ScVal) => U} parseResultXdr - Function to parse the XDR result returned by the network. |
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.
Reminder: these don't show up anywhere!
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.
Hopefully they show up in the generated html docs??
breaking changes removed in #992 |
Adds a
sign
andsend
function toAssembledTransaction
. Allows signing the transaction without sending.Addresses #979