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

cmd: allow min_chan_amt > amt for outbound liquidity orders #406

Merged
merged 2 commits into from
Nov 7, 2022

Conversation

positiveblue
Copy link
Contributor

Skip the extra check in the cli validation flow for the markets where this option is a valid.

This bug only applied to the validation logic in the cli package, poold and the backend services validate the orders correctly.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit 🎉

cmd/pool/order.go Outdated Show resolved Hide resolved
Comment on lines 213 to 216
// parseMinChanAmount parses and validates the value for the `min_chan_amt`
// parameter.
func parseMinChanAmount(ctx *cli.Context, amt uint64) (btcutil.Amount, error) {
minChanAmt := btcutil.Amount(ctx.Uint64("min_chan_amt"))
Copy link
Contributor

@ffranr ffranr Nov 1, 2022

Choose a reason for hiding this comment

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

I don't think this function does much parsing of min_chan_amt. It might be a good idea if this function was

validateMinChanAmount(minChanAmount btcutil.Amount, amt btcutil.Amount, auctionType) error

and move the getMinChanAmount functionality into a separate function. I think it would make more sense that a validator accepts amt as an argument. And testing might be simpler.

@guggero guggero linked an issue Nov 3, 2022 that may be closed by this pull request
@positiveblue positiveblue force-pushed the fix-402 branch 5 times, most recently from bdf41e5 to a26a812 Compare November 6, 2022 22:21
@positiveblue positiveblue requested a review from ffranr November 7, 2022 08:11
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Looks good! Nice refactoring.

I've only read the code. Let me know if I should do more.

cmd/pool/order.go Outdated Show resolved Hide resolved
cmd/pool/order.go Outdated Show resolved Hide resolved
cmd/pool/order.go Outdated Show resolved Hide resolved
cmd/pool/order.go Show resolved Hide resolved
cmd/pool/order.go Outdated Show resolved Hide resolved
Skip the extra check in the cli validation flow for the markets where
this option is a valid.

This bug only applied to the validation logic in the cli package, poold
and the backend services validate the orders correctly.
@positiveblue positiveblue merged commit ebd5ee0 into lightninglabs:master Nov 7, 2022
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.

Unable to create outbound ask order with minimum channel size
3 participants