work-in-progress! loopout: can set exact amount

[ratpoison4]

This was proposed here: #234 (comment)

I implemented logic of another Tx output here: sweep/sweeper.go
The logic of when to add which output is quite complex, so I added unit test for this (TestDeduceDestinations).
I am not familiar with the code, but I tried to pass everywhere new parameters. in OutRequest: DestAmount and ChangeAddr. In LoopOutQuoteRequest: WithChange. Careful review is required.

Also I don't know how to add an integration test and to test it in the wild (on testnet). Could you help me with this, please?

Pull Request Checklist

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

[ratpoison4]

Conflicts resolved and release_notes.md updated.

[guggero]

Gave this a high-level review because the author asked me to. Thanks for your work!

I like the idea of specifying a destination amount and using a change address for the rest. I think this is easier to implement than trying to scale the swap amount in order to send a specific amount to an address without change.

I do think this needs quite some work, though. I feel like there are quite a few edge cases lingering that need to be handled, especially in high fee environments.
And the database persistence of the parameters is also missing.

[guggero]

I think instead of returning three different fees and checking them all in loopout.go, this method should already decide what destination(s) to use and only return the fee for that. Otherwise this code is quite hard to follow with passing the data back and forth.

[guggero]

I'm not sure if silently failing here is good UX. Especially if the command line flag doesn't mention the behavior. Perhaps we should instead make sure that the swap amount is sufficiently larger than the destination amount?
Perhaps swap_amt >= max_sweep_fee + dest_amt?

[ratpoison4]

Fixed.

max_sweep_fee is actually known as max_miner_fee

[guggero]

This data also needs to be stored in the swap database. Otherwise if you restart before sweeping, it will be lost.

[ratpoison4]

Should deserializeLoopOutContract and serializeLoopOutContract be updated?

They use some custom binary format. I guess the change of the format should be backward-compatible, so that strings produced by old serializeLoopOutContract are successfully decoded by new deserializeLoopOutContract. I'll add two new fields in the end and in deserializeLoopOutContract if I get EOF trying to read the first field, I'll consider that ok, because it is an old format.

[ratpoison4]

if I get EOF trying to read the first field, I'll consider that ok, because it is an old format.

I see migration files. I think, I should add a migration.

[ratpoison4]

Done

[ratpoison4]

Found and fixed another issue: in loopOut when making QuoteRequest we should pass WithChange depending on dest_amount.

[guggero]

Concept ACK!

As explained on Slack, I think we do want this feature but are going to hold off on further review until the "work in progress" state is removed to hopefully reduce the number of review iterations needed.
Please take the following steps to continue:

• Split code into logical, incremental commit structure for easy review. each commit should have a rationale in the commit message and compile on its own
• Even though this isn't stated explicitly in the Loop docs, we do want contributors to stick to lnd's contribution guidelines, especially when it comes to code style and formatting: https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md
• Run the code on testnet to make sure it works end to end, comment the results on the PR
• Once all that is done, I think the "work in progress" can be removed from the title which also increases chances of getting more review

Thank you for your time and effort.

[lightninglabs-deploy]

@ratpoison4, remember to re-request review from reviewers for your latest update

[lightninglabs-deploy]

@ratpoison4, remember to re-request review from reviewers for your latest update

[guggero]

Closing due to inactivity.