lnrpc: added weight field to the transaction return for getTransactio…#10620
lnrpc: added weight field to the transaction return for getTransactio…#10620kushagra0902 wants to merge 1 commit intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the LND RPC interface by introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
lightninglabs-deploy
added
the
severity-high
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a weight field to the Transaction message, which is a useful addition for fee calculation. The changes to the protobuf definition and Swagger files are correct. I've identified a couple of areas for improvement in lnrpc/rpc_utils.go related to error handling, commenting, and code style, and have provided specific suggestions for them.
Note: Security Review is unavailable for this PR.
| if len(tx.RawTx) > 0 { | ||
| reader := bytes.NewReader(tx.RawTx) | ||
| var msgTx wire.MsgTx | ||
| if err := msgTx.Deserialize(reader); err == nil { | ||
| txWeight = blockchain.GetTransactionWeight( | ||
| btcutil.NewTx(&msgTx), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This new block for calculating transaction weight can be improved in two ways:
- Error Handling: The error from
msgTx.Deserialize(reader)is silently ignored. If deserialization fails,txWeightwill be 0, which could be misleading for the RPC caller. It's better to log this error to aid in debugging potential issues with transaction data. - Commenting: Per the LND Style Guide (lines 51-52), in-body comments should explain the intention of the code. This block would benefit from a comment explaining its purpose.
Here is a suggested implementation addressing both points. Note that you'll need to make the package logger available to use log.Errorf.
// If the raw transaction is available, we'll deserialize it to
// calculate its weight.
if len(tx.RawTx) > 0 {
reader := bytes.NewReader(tx.RawTx)
var msgTx wire.MsgTx
if err := msgTx.Deserialize(reader); err != nil {
// If we can't deserialize the transaction, we'll log
// it and return a weight of 0. This should not
// happen with valid data.
log.Errorf("unable to deserialize tx %v to get "+
"weight: %v", tx.Hash, err)
} else {
txWeight = blockchain.GetTransactionWeight(
btcutil.NewTx(&msgTx),
)
}
}References
- In-body comments should explain the intention of the code. (link)
| }, nil | ||
| } | ||
|
|
||
There was a problem hiding this comment.
This line contains unnecessary whitespace. It should be a blank line to separate logical code blocks, as per the style guide.
References
- Segment code into logical stanzas separated by newlines. (link)
|
@kushagra0902, remember to re-request review from reviewers when ready |
2 participants
Change Description
Added a "weight" field to the transactions returned by the RPCs like
listchaintxnsfor cli and other related RPC in gRPC and REST. This can be used to calculate fee_per_vbyte for unconfirmed transactions. This PR solves the issue #9833.Steps to Test
make rpcto ensure that the generated files are according to new definitions edited in protobuf file.make installto update the lnd and lncli binaries with new RPC definitions.regtestnetwork or any other network.lncli --network=<your_selected_network> listchaintxnsto get all the transactions. The listed transactions should have "Weights" field##Implementation Method
lnrpc/rpc_util.goin RPCTransaction function, usedblockchain.GetTransactionWeight()to get the weight involved and populated the corresponding field.