Skip to content

Mono-hop unidirectional payments#2

Merged
knocte merged 1 commit into
nblockchain:masterfrom
canndrew:geewallet-payments
May 18, 2020
Merged

Mono-hop unidirectional payments#2
knocte merged 1 commit into
nblockchain:masterfrom
canndrew:geewallet-payments

Conversation

@canndrew
Copy link
Copy Markdown

@canndrew canndrew commented Mar 16, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

knocte commented Apr 9, 2020

merge->squash

@knocte
Copy link
Copy Markdown
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
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
knocte
knocte reviewed Apr 14, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated

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
Copy Markdown
Member

@knocte knocte Apr 14, 2020

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
Copy Markdown
Member

@knocte knocte Apr 14, 2020

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)

knocte
knocte reviewed Apr 14, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
knocte
knocte reviewed Apr 14, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
knocte
knocte reviewed Apr 14, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
knocte
knocte reviewed Apr 14, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/Channel.fs
knocte
knocte reviewed Apr 14, 2020
View reviewed changes
Comment thread tests/DotNetLightning.Infrastructure.Tests/ChannelOperationTests.fs Outdated
@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
Comment thread .github/workflows/ci.yml Outdated
knocte
knocte reviewed Apr 17, 2020
View reviewed changes
Comment thread .github/workflows/ci.yml Outdated
@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
Comment thread src/DotNetLightning.Core/Channel/Channel.fs Outdated
knocte
knocte reviewed Apr 20, 2020
View reviewed changes
Comment thread .github/workflows/ci.yml Outdated
@knocte
Copy link
Copy Markdown
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
Comment thread .github/workflows/ci.yml Outdated
knocte
knocte reviewed Apr 21, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Serialize/Msgs/Msgs.fs
@canndrew canndrew force-pushed the geewallet-payments branch 4 times, most recently from d1edb32 to 7ab6e5c Compare April 21, 2020 10:08
knocte
knocte reviewed Apr 21, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
@canndrew canndrew force-pushed the geewallet-payments branch from 504ba35 to d0e55cb Compare April 28, 2020 09:14
knocte
knocte reviewed Apr 28, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Utils/LNMoney.fs Outdated
knocte
knocte reviewed Apr 28, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelTypes.fs Outdated
knocte
knocte reviewed Apr 28, 2020
View reviewed changes
Comment thread src/DotNetLightning.Core/Channel/ChannelTypes.fs Outdated
@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
Copy Markdown
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
Comment thread .github/workflows/publish_master.yml
cd $GITHUB_WORKSPACE/src/DotNetLightning.Core
dotnet pack . -p:Configuration=Release --version-suffix date`date +%Y%m%d-%H%M`-git-`echo $GITHUB_SHA | cut -c 1-7`-${{ matrix.RID }}
dotnet nuget push ./bin/Release/DotNetLightning.Core.1*.nupkg -k ${{ secrets.NUGET_API_KEY }} -s https://api.nuget.org/v3/index.json No newline at end of file
dotnet nuget push ./bin/Release/DotNetLightning.Kiss.1*.nupkg -k ${{ secrets.NUGET_API_KEY }} -s https://api.nuget.org/v3/index.json
Copy link
Copy Markdown
Member

@knocte knocte May 14, 2020

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
Copy Markdown
Author

@canndrew canndrew May 14, 2020

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
Comment thread 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
Copy Markdown
Member

@knocte knocte May 14, 2020

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
Copy Markdown
Author

@canndrew canndrew May 14, 2020

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
Copy Markdown
Member

@knocte knocte May 14, 2020

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
Copy Markdown
Author

@canndrew canndrew May 14, 2020

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
Comment thread src/DotNetLightning.Core/Serialize/Features.fs Outdated
static member Zero =
let b: bool array = [||]
b |> BitArray |> FeatureBit
static member Zero = FeatureBit()
Copy link
Copy Markdown
Member

@knocte knocte May 14, 2020

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
Copy Markdown
Author

@canndrew canndrew May 14, 2020

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
Comment thread src/DotNetLightning.Core/Channel/ChannelError.fs Outdated
Errors: string list
}
with
static member Create msg e = {
Copy link
Copy Markdown
Member

@knocte knocte May 14, 2020

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
Copy Markdown
Author

@canndrew canndrew May 14, 2020

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
Copy Markdown
Member

@knocte knocte May 14, 2020

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
Copy Markdown
Author

@canndrew canndrew May 14, 2020

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