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.

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

Mono-hop unidirectional payments #2

Merged
merged 1 commit into from
May 18, 2020
Merged

Mono-hop unidirectional payments #2

merged 1 commit into from
May 18, 2020

Conversation

canndrew
Copy link
Member

This is a work-in-progress PR for monodirectional, unihop (non-htlc) payments.

@canndrew canndrew mentioned this pull request Mar 16, 2020
Closed
@knocte
Copy link
Member

knocte commented Mar 16, 2020

Please:

  • rename any string that contains the substring GeewalletPayment to MonoHopUnidirectionalPayment.
  • remove "WIP" prefix from main commit (because it passes the build, therefore it implements what is needed for no revocation yet; it's the revocation_and_ack which is WIP, AFAIU).
  • revert the first commit about fsc.props to see if CI still passes without it.

@knocte
Copy link
Member

knocte commented Mar 16, 2020

Also comment out this line in ci.yml:

@canndrew canndrew force-pushed the geewallet-payments branch from b24eaca to a878176 Compare March 16, 2020 11:58
@canndrew canndrew changed the title WIP: monodirectional unihop payments Mono-hop unidirectional payments Mar 16, 2020
@canndrew canndrew force-pushed the geewallet-payments branch 5 times, most recently from f43bd8c to 4617a02 Compare March 23, 2020 10:45
@canndrew canndrew force-pushed the geewallet-payments branch from 4617a02 to 1353d6b Compare March 25, 2020 07:34
@knocte
Copy link
Member

knocte commented Apr 9, 2020

I guess you can merge the commits here (except for the ones being proposed to Joe or the hacky one)

@knocte
Copy link
Member

knocte commented Apr 9, 2020

merge->squash

@knocte
Copy link
Member

knocte commented Apr 9, 2020

@canndrew i've updated the tip of this repo's master to sync with Joe's, you can rebase

@canndrew canndrew force-pushed the geewallet-payments branch 2 times, most recently from c42e57d to 0e90ded Compare April 14, 2020 12:06
knocte
knocte reviewed Apr 14, 2020
View reviewed changes
src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
@@ -458,6 +470,18 @@ module internal AcceptChannelMsgValidation =

(check1 |> Validation.ofResult) *^> check2 *^> check3 *^> check4 *^> check5 *^> check6 *^> check7

module UpdateMonoHopUnidirectionalPaymentWithContext =
let internal checkWeHaveSufficientFunds (state: Commitments) (currentSpec) =
let fees = if (state.LocalParams.IsFunder) then (Transactions.commitTxFee (state.RemoteParams.DustLimitSatoshis) currentSpec) else Money.Zero
Copy link
Member

Choose a reason for hiding this comment

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

please split this long line in the same way we do in geewallet:

let foo =
    if bar then
        baz
    else
        bazz

Copy link
Member

Choose a reason for hiding this comment

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

(also no need for so many parenthesis)

@canndrew canndrew force-pushed the geewallet-payments branch 2 times, most recently from a0dba66 to 0e90ded Compare April 16, 2020 09:23
knocte
knocte reviewed Apr 17, 2020
View reviewed changes
knocte
knocte reviewed Apr 17, 2020
View reviewed changes
@canndrew canndrew force-pushed the geewallet-payments branch from 0e90ded to 06726a3 Compare April 20, 2020 07:13
knocte
knocte reviewed Apr 20, 2020
View reviewed changes
@knocte
Copy link
Member

knocte commented Apr 21, 2020

might try to send the last commit (Make GetFailureMsgData format non-printable error messages) upstream though.

ok then separate that one but let's not delay this so much

@canndrew canndrew force-pushed the geewallet-payments branch from 6f88df3 to 7f242df Compare April 21, 2020 08:08
knocte
knocte reviewed Apr 21, 2020
View reviewed changes
@canndrew canndrew force-pushed the geewallet-payments branch 4 times, most recently from d1edb32 to 7ab6e5c Compare April 21, 2020 10:08
@canndrew canndrew force-pushed the geewallet-payments branch from 504ba35 to d0e55cb Compare April 28, 2020 09:14
@canndrew canndrew force-pushed the geewallet-payments branch 4 times, most recently from 1d27824 to 698ea77 Compare May 6, 2020 09:09
@knocte
Copy link
Member

knocte commented May 13, 2020

@canndrew pls rebase when u have time

@canndrew canndrew force-pushed the geewallet-payments branch from 698ea77 to 7fc86f6 Compare May 14, 2020 08:50
knocte
knocte reviewed May 14, 2020
View reviewed changes
.github/workflows/publish_master.yml
dotnet nuget push ./bin/Release/DotNetLightning.Kiss.1*.nupkg -k ${{ secrets.NUGET_API_KEY }} -s https://api.nuget.org/v3/index.json
Copy link
Member

Choose a reason for hiding this comment

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

this looks wrong, we should push BouncyCastle binaries, not native

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. I've fixed it so that we only upload the BouncyCastle build and we upload it as DotNetLightning.Kiss

knocte
knocte reviewed May 14, 2020
View reviewed changes
src/DotNetLightning.Core/Channel/Commitments.fs Outdated
let totalBalance = reduced.ToRemote
let untrimmedSpendableBalance = totalBalance - channelReserve - fees
let htlcSuccessFee =
reduced.FeeRatePerKw.ToFee(Transactions.Constants.HTLC_SUCCESS_WEIGHT)
Copy link
Member

Choose a reason for hiding this comment

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

why do we use any HTLC-related stuff here if we don't use HTLCs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I had to think hard about how exactly this function needs to be implemented and I didn't want to have to do that again, or to have the code silently break, when we eventually add support for HTLCs. So I just made it HTLC-compatible to begin with.

Copy link
Member

Choose a reason for hiding this comment

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

that's great but I'd prefer if you move this code into another PR, to pick it up later

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the HTLC fees from the calculation and created a seperate PR which adds them back here: #3

knocte
knocte reviewed May 14, 2020
View reviewed changes
src/DotNetLightning.Core/Serialize/Features.fs Outdated
static member Zero =
let b: bool array = [||]
b |> BitArray |> FeatureBit
static member Zero = FeatureBit()
Copy link
Member

Choose a reason for hiding this comment

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

is this change really needed for this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. I can remove it.

@canndrew canndrew force-pushed the geewallet-payments branch from d054b8b to 3a59364 Compare May 14, 2020 09:16
knocte
knocte reviewed May 14, 2020
View reviewed changes
src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
Errors: string list
}
with
static member Create msg e = {
Copy link
Member

Choose a reason for hiding this comment

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

hey let's remove this Create method, as the record can be instantiated easily anyway without it

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do. All the other invalid msg types have it though, though they all seem equally pointless. Is it important to be consistent here?

Copy link
Member

Choose a reason for hiding this comment

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

good point; let's remove it here and I'll prepare a PR to remove them upstream

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@canndrew canndrew force-pushed the geewallet-payments branch from 3a59364 to e8beb8e Compare May 14, 2020 10:16
@canndrew canndrew mentioned this pull request May 14, 2020
Open
@canndrew canndrew force-pushed the geewallet-payments branch from e8beb8e to bbc27ea Compare May 18, 2020 07:47
@knocte knocte merged commit 8195bd4 into nblockchain:master May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knocte knocte knocte left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@canndrew @knocte