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

Improve coin selection unit tests #823

Merged
merged 4 commits into from
Mar 9, 2024
Merged

Improve coin selection unit tests #823

merged 4 commits into from
Mar 9, 2024

Conversation

guggero
Copy link
Member

@guggero guggero commented Mar 6, 2024

Fixes #124.

@guggero guggero requested review from ffranr and jharveyb March 6, 2024 15:51
@jharveyb
Copy link
Contributor

jharveyb commented Mar 7, 2024

Found some issues (in the unit tests) revealed by these test changes.

Put up a branch that could get pulled into this PR: https://github.com/lightninglabs/taproot-assets/tree/coin-selection-test-fixes

Looks good otherwise!

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

See above comment; the coin selection fix was waiting on the wrong signal; IIUC the timeout would occur for any signal since the method call was complete.

The second fix was to actually have the second asset as a reissuance into the first asset group; IIUC the test as-is was creating a duplicate asset.

@guggero
Copy link
Member Author

guggero commented Mar 8, 2024

Thanks for the review and the fixes. I took over the commit with the group anchor improvement.

IIUC the timeout would occur for any signal since the method call was complete.

That was kind of on purpose. We explicitly expect the LeaseCoins method not to be called. I agree that the timeout would occur on any other channel as well. But IMO selecting on the listSignals a second time doesn't really change that?

@jharveyb
Copy link
Contributor

jharveyb commented Mar 8, 2024

That was kind of on purpose. We explicitly expect the LeaseCoins method not to be called. I agree that the timeout would occur on any other channel as well. But IMO selecting on the listSignals a second time doesn't really change that?

Ah, fair enough - so any of the 3 calls timing out is sufficient.

@jharveyb jharveyb self-requested a review March 8, 2024 15:21
Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

LGTM, coverage go up 🚀

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@Roasbeef Roasbeef added this pull request to the merge queue Mar 9, 2024
Merged via the queue into main with commit 761ccca Mar 9, 2024
14 checks passed
@guggero guggero deleted the coin-selection-tests branch March 11, 2024 06:29
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.

tarodb: add additional unit test coverage for asset coin selection
3 participants