-
Notifications
You must be signed in to change notification settings - Fork 575
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
adds missing fields to wallet transaction RPCs #3768
Conversation
adds transaction fields needed for developers to determine transaction status independently of the node - adds blockHash and blockSequence to wallet/getAccountTransactions - adds submittedSequence to responses for both wallet/getAccountTransaction and wallet/getAccountTransactions - adds confirmations to wallet/getAccountTransaction and wallet/getAccountTransactions
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.
curl -d '{"hash": "2f5c7e1bf5aece4ffc2b85949736c9974a494e31cd1e724e44769a514d57e0f8"}' -X GET http://localhost:8021/wallet/getAccountTransaction
{"status":200,"data":{"account":"default","transaction":{"hash":"2f5c7e1bf5aece4ffc2b85949736c9974a494e31cd1e724e44769a514d57e0f8","fee":"2","blockHash":"0000000000028d26f59647523430f06771d7c647cdd923bc94123acb935a0d1a","blockSequence":53908,"notesCount":2,"spendsCount":4,"mintsCount":0,"burnsCount":0,"expiration":53921,"timestamp":1681146498143,"submittedSequence":53908,"assetBalanceDeltas":[{"assetId":"d7c86706f5817aa718cd1cfad03233bcd64a7789fd9422d3b17af6823a7e6ac6","assetName":"2449524f4e","delta":"50000"}],"notes":[{"isOwner":true,"owner":"e0ed4dc14e6591595da1bdaf5f8acb7c01bc8687755bb71e59ea05264e2e5db7","memo":"","value":"50000","assetId":"d7c86706f5817aa718cd1cfad03233bcd64a7789fd9422d3b17af6823a7e6ac6","assetName":"2449524f4e","sender":"89f22093001943e7885ef9ead1c5b05a9fc6cdf7d36fd5b070b328555fba20ce","spent":false}],"status":"confirmed","type":"receive","confirmations":2}}}
@@ -106,7 +114,7 @@ router.register<typeof GetAccountTransactionsRequestSchema, GetAccountTransactio | |||
|
|||
const options = { | |||
headSequence, | |||
confirmations: request.data.confirmations, | |||
confirmations: request.data.confirmations ?? node.config.get('confirmations'), |
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.
you don't need to materialize this here right? it gets materialized where its used farther down
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.
Same as above -- we only need to materialize this here to include it in the response
@@ -118,8 +122,10 @@ router.register<typeof GetAccountTransactionRequestSchema, GetAccountTransaction | |||
|
|||
const notes = await getAccountDecryptedNotes(node, account, transaction) | |||
|
|||
const confirmations = request.data.confirmations ?? node.config.get('confirmations') |
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.
getTransactonStatus already materializes this so I dont think we need this code in the RPC do we?
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'm materializing it here only to include it in the response
Summary
adds transaction fields needed for developers to determine transaction status independently of the node
adds blockHash and blockSequence to wallet/getAccountTransactions
adds submittedSequence to responses for both wallet/getAccountTransaction and wallet/getAccountTransactions
adds confirmations to wallet/getAccountTransaction and wallet/getAccountTransactions
orders fields consistently between endpoints
Testing Plan
Documentation
Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.