-
Notifications
You must be signed in to change notification settings - Fork 123
staticaddr: show deposits and swap hashes in cli #991
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
staticaddr: show deposits and swap hashes in cli #991
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.
Summary of Changes
Hello @hieblmi, 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 significantly enhances the command-line interface (CLI) for static address swaps and deposits by integrating more comprehensive data. Previously, certain details were not readily available. The changes introduce the display of associated deposit information and swap hashes directly within the CLI output, making it easier for users to track and understand the state of their static address operations. This improves the overall observability and user experience without altering core swap or deposit functionalities.
Highlights
- Enhanced Static Address Swap Details: The
ListStaticAddressSwaps
RPC now includes detailed deposit information for each static address loop-in swap. This is achieved by fetching all deposits and associating them with their respective swaps using theSwapHash
. - Deposit-to-Swap Linkage: The
Deposit
message now contains aSwapHash
field, directly linking individual deposits to the static address loop-in swaps they are part of. This provides a clearer relationship between deposits and swaps. - Database Performance Improvements: New database indices have been added to the
deposits
anddeposit_updates
tables. These indices (deposits_id_index
anddeposit_updates_id_timestamp_index
) will improve the performance of database queries related to deposits.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request enhances the CLI by showing deposits and swap hashes, which is a great improvement for user visibility. The changes are well-implemented across the database schema, protobuf definitions, and server logic. I've identified one area for improvement to ensure API consistency: the BlocksUntilExpiry
field for deposits within a swap is not populated. I've added comments with suggestions to address this.
loopd/swapclient_server.go
Outdated
pd := &looprpc.Deposit{ | ||
Id: d.ID[:], | ||
State: toClientDepositState(d.GetState()), | ||
Outpoint: d.OutPoint.String(), | ||
Value: int64(d.Value), | ||
ConfirmationHeight: d.ConfirmationHeight, | ||
SwapHash: d.SwapHash, | ||
} |
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.
The BlocksUntilExpiry
field is not being set for the deposits associated with a swap. This is inconsistent with the ListStaticAddressDeposits
endpoint where this field is populated. This field should be calculated using the bestBlockHeight
and expiry
variables.
pd := &looprpc.Deposit{
Id: d.ID[:],
State: toClientDepositState(d.GetState()),
Outpoint: d.OutPoint.String(),
Value: int64(d.Value),
ConfirmationHeight: d.ConfirmationHeight,
SwapHash: d.SwapHash,
BlocksUntilExpiry: d.ConfirmationHeight + expiry - bestBlockHeight,
}
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.
Shall we fill BlocksUntilExpiry
?
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 wasn't sure about the applicability of this field since it shouldn't provide value after a deposit is swapped. But I guess we can add it.
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.
added
ef480c6
to
d068e9e
Compare
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 good, just a few nits!
loopd/swapclient_server.go
Outdated
pd := &looprpc.Deposit{ | ||
Id: d.ID[:], | ||
State: toClientDepositState(d.GetState()), | ||
Outpoint: d.OutPoint.String(), | ||
Value: int64(d.Value), | ||
ConfirmationHeight: d.ConfirmationHeight, | ||
SwapHash: d.SwapHash, | ||
} |
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.
Shall we fill BlocksUntilExpiry
?
d068e9e
to
0b699c2
Compare
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 pending comments 💾
0b699c2
to
8d04f67
Compare
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, pending build fix 🎉
8d04f67
to
1bacf26
Compare
staticaddr/deposit/sql_store.go
Outdated
@@ -281,6 +290,7 @@ func ToDeposit(row sqlc.Deposit, lastUpdate sqlc.DepositUpdate) (*Deposit, | |||
ConfirmationHeight: row.ConfirmationHeight, | |||
TimeOutSweepPkScript: row.TimeoutSweepPkScript, | |||
ExpirySweepTxid: expirySweepTxid, | |||
SwapHash: &swapHash, |
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.
It should be nil
if row.SwapHash == nil
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.
To avoid the pointer pitfalls I've changed the declaration of SwapHash
to lntypes.Hash
instead of a pointer.
loopd/swapclient_server.go
Outdated
|
||
depositsBySwap := make(map[lntypes.Hash][]*deposit.Deposit, len(swaps)) | ||
for _, d := range allDeposits { | ||
if len(d.SwapHash) == 0 { |
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.
if d.SwapHash == nil
It's a pointer to lntypes.Hash
now, not a byte slice.
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.
len(d.SwapHash)
always return the length of lntypes.Hash array type.
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 think this code below panics if d.SwapHash is nil:
pd := &looprpc.Deposit{
Id: d.ID[:],
State: state,
Outpoint: d.OutPoint.String(),
Value: int64(d.Value),
ConfirmationHeight: d.ConfirmationHeight,
SwapHash: d.SwapHash[:],
BlocksUntilExpiry: blocksUntilExpiry,
}
d.SwapHash[:]
is segfault.
Could you add an test/itest to safeguard against this in the future, please?
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.
added a unit-test.
staticaddr/deposit/sql_store.go
Outdated
@@ -270,6 +271,14 @@ func ToDeposit(row sqlc.Deposit, lastUpdate sqlc.DepositUpdate) (*Deposit, | |||
} | |||
} | |||
|
|||
var swapHash lntypes.Hash |
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.
var swapHash *lntypes.Hash
701b631
to
574ca7a
Compare
|
||
/* | ||
The swap hash of the swap that this deposit is part of. This field is only | ||
set if the deposit is part of a loop-in swap. |
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 field is empty inside looprpc.ListStaticAddressWithdrawalResponse
but it is an array of zero bytes inside looprpc.ListStaticAddressDepositsResponse
for deposits not associated with a swap.
Function filter
in loopd/swapclient_server.go
sets it to an array of zero bytes if it is ZeroHash. It is called e.g. to fill looprpc.ListStaticAddressDepositsResponse
proto message.
Can we update the code to always produce an empty slice for this field in proto for consistency? I propose to verify this with a test.
staticaddr/deposit/sql_store_test.go
Outdated
} | ||
} | ||
|
||
func dummyTxHashBytes() []byte { |
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.
s/dummyTxHashBytes/dummyHashBytes/
It is not a tx hash.
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.
changed the name
574ca7a
to
b2a4d7a
Compare
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! 🏗
I tested this PR manually with loop static listdeposits
. It works as intended! It provides swap hashes for swap associated deposits and "swap_hash": ""
otherwise.
This PR adds displaing the swap hash in listdeposits and the used deposits in listswaps.
Examples:
loop static listswaps
:loop static listdeposits
: