Skip to content

staticaddr: method to fetch deposits by outpoints #959

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hieblmi
Copy link
Collaborator

@hieblmi hieblmi commented Jun 17, 2025

This PR adds method DepositsForOutpoints to the deposit manager and required sub-methods to the deposit store.

This PR was extracted from and is required by #887 to ease review.

@hieblmi hieblmi self-assigned this Jun 17, 2025
@hieblmi hieblmi requested review from starius and sputn1ck June 17, 2025 12:12
@hieblmi hieblmi force-pushed the deposit-outpoint branch 2 times, most recently from 5f5780d to 1cc4bbb Compare June 17, 2025 18:02
@hieblmi hieblmi force-pushed the deposit-outpoint branch from 1cc4bbb to 824ea43 Compare June 24, 2025 08:20
@bhandras bhandras requested review from bhandras and removed request for sputn1ck June 26, 2025 11:13
outpoints []string) ([]*Deposit, error) {

// Check for duplicates.
duplicates := make(map[string]struct{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i'd call this existingOutpoints.


// DepositsForOutpoints returns all deposits that are behind the given
// outpoints.
func (m *Manager) DepositsForOutpoints(ctx context.Context,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would we use the function? Could you please add coverage?

Copy link
Collaborator Author

@hieblmi hieblmi Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, it is used to restore deposits of a swap after fetching it from the db, see hieblmi@349b930#diff-2c26627ef62d3f06764cb007a83924ef8abb36066a183b4d05e7f42e7c6a7ac9R23-R283

Ideally this would all be handled by GetLoopInByHash, but it isn't currently.

@hieblmi hieblmi force-pushed the deposit-outpoint branch from 824ea43 to bed8b40 Compare June 26, 2025 19:25
Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 💾
Few comments.

@@ -26,6 +27,10 @@ type Store interface {
// GetDeposit retrieves a deposit with depositID from the database.
GetDeposit(ctx context.Context, depositID ID) (*Deposit, error)

// DepositForOutpoint retrieves the deposit with the given outpoint.
DepositForOutpoint(ctx context.Context, txHash chainhash.Hash,
idx uint32) (*Deposit, error)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to change the signature to more high level:

DepositForOutpoint(ctx context.Context, outpoint string) (*Deposit, error)

outpoints []string) ([]*Deposit, error) {

// Check for duplicates.
duplicates := make(map[string]struct{})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to pre-allocate the map. We now the max size.

Comment on lines +164 to +178
row, err := q.DepositForOutpoint(ctx, params)
if err != nil {
return err
}

latestUpdate, err := q.GetLatestDepositUpdate(
ctx, row.DepositID,
)
if err != nil {
return err
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do it with SQL join and avoid making an explicit DB transaction (ExecTx).

@hieblmi hieblmi force-pushed the deposit-outpoint branch 2 times, most recently from 78bc133 to a3531a9 Compare June 27, 2025 19:52
@hieblmi hieblmi force-pushed the deposit-outpoint branch from a3531a9 to 25c90b0 Compare July 2, 2025 17:01
@hieblmi
Copy link
Collaborator Author

hieblmi commented Jul 2, 2025

Rebased master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants