Skip to content

liquidity: add loopin to autoloop #419

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.

Sign up for GitHub

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

Jump to bottom
Merged
arshbot merged 9 commits into from
Dec 15, 2021
Merged

liquidity: add loopin to autoloop #419

arshbot merged 9 commits into from
Dec 15, 2021

Conversation

@carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Sep 7, 2021 •
edited
Loading

This PR updates autoloop to add support for loop in swaps.

Pull Request Checklist

  • Update release_notes.md if your PR contains major features, breaking changes or bugfixes

@carlaKC carlaKC marked this pull request as ready for review November 3, 2021 10:20
@carlaKC carlaKC requested review from arshbot and bhandras November 3, 2021 10:20
@carlaKC
Copy link
Contributor Author

carlaKC commented Nov 3, 2021

There are some tests needed here still, but wanted to get a first round of review before I fill everything out.

Thinking of changing the style of test in the liquidity subsystem. In the past we've gone for black box tests, driving everything through SuggestSwaps rather than more traditional unit tests of each components. IMO that has made unit testing quite "heavy", because we have to set up channels/ parameters/ expected outcomes for each unit test. Would be interested in thoughts here.

@carlaKC
Copy link
Contributor Author

carlaKC commented Nov 11, 2021

@arshbot @bhandras PTAL 🙏

@carlaKC carlaKC changed the title [WIP]: add loopin to autoloop liquidity: add loopin to autoloop Nov 12, 2021
@carlaKC
Copy link
Contributor Author

carlaKC commented Nov 12, 2021 •
edited
Loading

hold off on review, want to push a few more changes!

✅ done

@carlaKC carlaKC mentioned this pull request Nov 15, 2021
Merged
1 task
@carlaKC carlaKC force-pushed the autoloop-4-loopin branch 2 times, most recently from 7b1bdc0 to 44017a5 Compare December 2, 2021 07:28
@carlaKC
Copy link
Contributor Author

carlaKC commented Dec 2, 2021

Rebased on dependent PR. I'm going to add a bunch more tests here to make sure there's no freaky interaction when we have autoloop-out and autoloop-in both enabled.

Would appreciate a first pass on approach before I delve too deep @bhandras, @arshbot 🙏

bhandras
bhandras reviewed Dec 3, 2021
View reviewed changes
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Looks really good!

@carlaKC carlaKC force-pushed the autoloop-4-loopin branch 2 times, most recently from 81eaede to f40d138 Compare December 7, 2021 11:30
@carlaKC
Copy link
Contributor Author

carlaKC commented Dec 7, 2021

Diff from last review here, majority of change is more tests.

@carlaKC carlaKC requested a review from bhandras December 7, 2021 11:40
@lightninglabs-deploy
Copy link

lightninglabs-deploy commented Dec 7, 2021

@bhandras: review reminder

bhandras
bhandras approved these changes Dec 7, 2021
View reviewed changes
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, not even a nit 🥇

@lightninglabs-deploy
Copy link

lightninglabs-deploy commented Dec 10, 2021

@arshbot: review reminder

1 similar comment
@lightninglabs-deploy
Copy link

lightninglabs-deploy commented Dec 13, 2021

@arshbot: review reminder

arshbot
arshbot suggested changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@arshbot arshbot left a comment

Choose a reason for hiding this comment

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

Looks great! I left one comment that I'm just not sure on, but other than that -- echo what @bhandras said :)

@carlaKC carlaKC requested a review from arshbot December 15, 2021 07:00
@arshbot arshbot merged commit be6eae3 into lightninglabs:master Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bhandras bhandras bhandras approved these changes

+1 more reviewer

@arshbot arshbot arshbot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@carlaKC @lightninglabs-deploy @bhandras @arshbot