liquidity: flat fee percentage for autoloop #340
Conversation
carlaKC
force-pushed
the
autoloop-feepercentage
branch
from
9308efd to
79764bd
Compare
carlaKC
changed the title
There was a problem hiding this comment.
tACK, this is a great simplification for the user!
The only thing I ran into when testing this on regtest is that the check of the m.params.SweepFeeRateLimit complained when I only set the --autoloop param and left everything else at the defaults. Not sure if it would make sense to bypass that check if we're using the proportional fees? Since we look at the miner fees there too.
liquidity/fees.go
Outdated
| MaximumRoutingFeePPM: routingFeePPM, | ||
| MaximumPrepayRoutingFeePPM: prepayFeePPM, | ||
| MaximumMinerFee: minerFee, | ||
| MaximumPrepay: prepay, |
There was a problem hiding this comment.
style/nit: could be in the same order as defined in the struct
|
|
||
| // FeePortion is a fee limitation which limits fees to a set portion of | ||
| // the swap amount. | ||
| type FeePortion struct { |
There was a problem hiding this comment.
Why not simply type FeePortionPPM int?
There was a problem hiding this comment.
Just personal style preference, I think that f.PartsPerMillion is more readable than *f when we're using the value in various funcs.
liquidity/fees.go
Outdated
| ) | ||
|
|
||
| return prepayMaxFee, routeMaxFee, quote.MinerFee | ||
|
|
| } | ||
|
|
||
| if quote.SwapFee > feeLimit { | ||
| log.Debugf("swap fee: %v greater than fee limit: %v, at "+ |
There was a problem hiding this comment.
This was debated in a previous PR and debug was decided on. I wanted info at the time, but now that we have reasons surfaced to the user, I think it's ok to leave "additional info" on a lower debug level.
carlaKC
force-pushed
the
autoloop-feepercentage
branch
2 times, most recently
from
b6d9b7c to
b877f7c
Compare
carlaKC
force-pushed
the
autoloop-feepercentage
branch
from
b877f7c to
9d845dd
Compare
Yeah planning on updating the itest with the new client version anyway, so will add to the existing test! |
carlaKC
force-pushed
the
autoloop-feepercentage
branch
from
9d845dd to
34726a4
Compare
|
Tried this out on testnet and realized that we need to pad max miner fee so that we'll continue with swaps even if fees spike after initiating. Relatively small diff, but just wanted to make reviewers aware @guggero @bhandras. This should be addressed when we figure out a better way of dealing with the max miner issue, current approach is just to do as we do for regular swaps. |
Need to add some unit tests.
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes