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

config: increase max funding and payment amount 60x under Litecoin #1246

Merged
merged 1 commit into from
May 26, 2018

Conversation

wpaulino
Copy link
Contributor

No description provided.

@wpaulino wpaulino added this to the 0.4.2-beta milestone May 14, 2018
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.

This also missed increasing the max payment size.

@wpaulino wpaulino changed the title fundingmanager+config: increase max funding amount 60x under Litecoin config: increase max funding and payment amount 60x under Litecoin May 15, 2018
@wpaulino wpaulino force-pushed the bump-ltc-limits branch 2 times, most recently from f90cba3 to 3adc923 Compare May 15, 2018 03:10
// primary chain due to the price difference.
//
// TODO(roasbeef): add command line param to modify
maxFundingAmount = btcutil.Amount(1 << 24)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it'd be preferable to have enumerate both constants, maxBtcFundingAmount and maxLtcFundingAmount similar to how we have for the other constraints.

If we want to make this easy to modify in the future, we can also define something like btcToLtcConversionRate = 60 and compute ltc constants at compile time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

config.go Outdated
registeredChains.RegisterPrimaryChain(litecoinChain)
maxFundingAmount *= 60
maxPaymentMSat *= 60
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this scaling here, we can set local maxFundingAmount or maxPaymentMSat vars based on which chain is active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@wpaulino wpaulino force-pushed the bump-ltc-limits branch 3 times, most recently from 13cf1f1 to 0b4287e Compare May 23, 2018 19:48
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

Awesome, this looks great @wpaulino! One question I have is: should we update the min channel size as well? @Roasbeef might have some insight into this.

)

var (
// maxBtcFundingAmount is a soft-limit of the maximum channel size
Copy link
Contributor

Choose a reason for hiding this comment

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

s/maxBtcFundingAmount/maxFundingAmount/

Copy link
Member

Choose a reason for hiding this comment

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

👆🏾

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

)

var (
// maxBtcFundingAmount is a soft-limit of the maximum channel size
Copy link
Member

Choose a reason for hiding this comment

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

👆🏾

// Protocol. This limit is defined in BOLT-0002, and serves as an
// initial precautionary limit while implementations are battle tested
// in the real world.
maxBtcFundingAmount = btcutil.Amount(1 << 24)
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, this should actually be -1 at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

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 merged commit fa5c0b9 into lightningnetwork:master May 26, 2018
@wpaulino wpaulino deleted the bump-ltc-limits branch May 26, 2018 02:50
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.

None yet

3 participants