-
Notifications
You must be signed in to change notification settings - Fork 217
fix(rpc): add missing signature field to SignAndSendTransactionResponse #270
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
fix(rpc): add missing signature field to SignAndSendTransactionResponse #270
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.
Important
Looks good to me! 👍
Reviewed everything up to afea55f in 35 seconds. Click for details.
- Reviewed
25lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sdks/ts/src/types/index.ts:112
- Draft comment:
Added the missing 'signature' field to SignAndSendTransactionResponse. Looks correct; ensure your RPC parser correctly extracts this field from responses. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. sdks/ts/test/unit.test.ts:271
- Draft comment:
Updated signAndSendTransaction test to include the 'signature' field, which now matches the updated type. Test reflects expected outcome. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_hQGSlGZp6yeLrWXs
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Hey ! thanks for the PR, can you fix the CI errors (typescript testS) and i'll be happy to merge |
|
Hey @dev-jodee! Thanks for the review 🙌 |
|
Thanks for that, actually just realized what you're trying to do, and you'll need a quick change in rust as well, right now you only changed the TS types but the backend itself won't return the signature, there's a bit of work to be done here : https://github.com/solana-foundation/kora/blob/main/crates/lib/src/rpc_server/method/sign_and_send_transaction.rs#L29 :) |
|
Thanks for pointing that out. The internal flow already returned the signature, I've updated the Rust implementation to include it in the response, synced the |
dev-jodee
left a comment
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.
lgtm
Summary
This PR adds the missing
signaturefield to theSignAndSendTransactionResponse.The field is required for clients that need to log, verify, or process transaction signatures returned from
signAndSendTransaction.Changes
signature: stringtoSignAndSendTransactionResponsesignaturefield is returned correctlyTests
Documentation
No documentation changes required — the API documentation already includes the
signaturefield.Motivation
Without the
signaturefield, consumers ofsignAndSendTransactioncould not access the resulting transaction signature, preventing verification and logging workflows.This update aligns the Kora RPC response with expected Solana client behaviors.
Type of Change
fix)feat)!)docs)refactor)chore)Important
Add missing
signaturefield toSignAndSendTransactionResponseand update tests to ensure correct inclusion.signature: stringtoSignAndSendTransactionResponseinindex.ts.signaturefield.unit.test.tsto ensuresignaturefield is returned correctly insignAndSendTransaction.signaturefield.This description was created by
for afea55f. You can customize this summary. It will automatically update as commits are pushed.