Skip to content
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

Collect small fixes from stale contributor PRs #969

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

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Dec 9, 2024

Replaces #959.
Replaces #824.
Replaces #928.

Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

lgtm! thanks 🙏 a couple of linter errors

Comment on lines 1722 to 1718
if result == nil {
return nil, errUtxoSpent
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok cool - i guess ideally we could change Receive to return an error if the result is nil and then dont need to explicitly handle this. but i see we already do this special case handling in lnd for all calls to Receive done via GetTxOut.

Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Looks like the liner is complaining - also can we open a PR in lnd to test this change?

go.mod Outdated
@@ -21,6 +21,7 @@ require (
github.com/lightninglabs/gozmq v0.0.0-20191113021534-d20a764486bf
github.com/lightninglabs/neutrino v0.16.0
github.com/lightninglabs/neutrino/cache v1.1.2
github.com/lightningnetwork/lnd/fn v1.2.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

didn't realize we were importing modules from lnd before😮‍💨

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. But those are all submodules, so it shouldn't really matter. But noticed this actually needs to be github.com/lightningnetwork/lnd/fn/v2, so I changed that.

@guggero guggero force-pushed the small-pr-consolidation branch from 8412709 to 2a436f5 Compare December 10, 2024 17:44
guggero added a commit to guggero/lnd that referenced this pull request Dec 10, 2024
Depends on btcsuite/btcwallet#969.
Makes sure the changes in that small PR don't cause any failures in lnd.
Chinwendu20 and others added 2 commits December 10, 2024 18:49
We disable some of the linters that give us too many false positives.
Those linters are also disabled in other projects such as lnd for
example.
@guggero guggero force-pushed the small-pr-consolidation branch from 2a436f5 to 3b576ed Compare December 10, 2024 17:49
guggero added a commit to guggero/lnd that referenced this pull request Dec 10, 2024
Depends on btcsuite/btcwallet#969.
Makes sure the changes in that small PR don't cause any failures in lnd.
@guggero
Copy link
Collaborator Author

guggero commented Dec 10, 2024

lnd PR is here: lightningnetwork/lnd#9346.

@guggero guggero requested a review from yyforyongyu December 10, 2024 18:17
@yyforyongyu
Copy link
Collaborator

hmm think lnd CI is broken...

@guggero
Copy link
Collaborator Author

guggero commented Dec 11, 2024

Yeah, I'll take a look at some point. Seems like the early rescan abort shortcut breaks things. Perhaps because we re-use existing rescans, but then it's already aborted?

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

Successfully merging this pull request may close these issues.

6 participants