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

docs(0023): unconditional transfer type for ilp4 #360

Merged
merged 4 commits into from
Mar 5, 2018

Conversation

sharafian
Copy link

Adds unconditional transfer type to accommodate #359 . Related to interledgerjs/btp-packet#18 .

Copy link
Member

@justmoon justmoon left a comment

Choose a reason for hiding this comment

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

LGTM, but let's not merge until everything actually uses this.

@michielbdejong
Copy link
Contributor

Wouldn't it be more common for the party who does the on-ledger or on-paychan payment, to send a BTP message saying "My BTP balance just went up because I paid you on-ledger, look here are the details" instead of what currently seems to be a call type to notify your peer that "Your BTP balance just went up because you paid me on-ledger, look here are the details"

@michielbdejong
Copy link
Contributor

michielbdejong commented Jan 5, 2018

@sharafian see also #347 (comment)

The term "Transfer" implies that when you use this call type, your BTP balance will go down. The way I understood plugin.sendMoney is that it would make your BTP balance go up? Or are we removing the concept of BTP balance altogether?

@sharafian
Copy link
Author

sendMoney makes your balance go down, because you're sending money to your peer. A BTP connection/relationship no longer has a balance, but the Transfer call is used to carry information about an on-ledger (in this context payment channels count as "on ledger" because they're secured by the ledger) transfer. I believe it's pretty similar to your vouch protocol in that way

@michielbdejong
Copy link
Contributor

A BTP connection/relationship no longer has a balance, but the Transfer call is used to carry information

If you have one call registry where Prepare sends a conditional transfer, and Transfer sends an unconditional one, then it would be super-confusing, if their amounts work in opposite directions (Prepare conditionally increases the sender's debt, Transfer unconditionally lowers that same debt)

@michielbdejong
Copy link
Contributor

How about 'Settle' instead of 'Transfer'?

@sharafian
Copy link
Author

Transfer and Prepare aren't in the same call registry though. From the plugin's point of view, sendData just sends data. The Transfer call is the only one that involves money.

@sharafian
Copy link
Author

I wouldn't be hugely opposed to Settle instead of Transfer, but having to refactor around naming is always a little annoying.

@michielbdejong
Copy link
Contributor

Transfer and Prepare aren't in the same call registry

They are, call types 3 and 7, see https://github.com/interledger/rfcs/pull/360/files#diff-2c87a4d4e57dd9430f40e0fe2b4d295aR114

@michielbdejong
Copy link
Contributor

michielbdejong commented Jan 5, 2018

sendData is the LPI method which causes a Prepare to be sent, it increases the sender's debt.
sendMoney is the LPI method which causes a Transfer Settle to be sent, it decreases the sender's debt.

@emschwartz
Copy link
Member

sendData is the LPI method which causes a Prepare to be sent, it increases the sender's debt.

Wouldn't it be an IlpPrepare sent over a BTP Type 6 Message? I don't think we'll be using the BTP Prepare anymore

@michielbdejong
Copy link
Contributor

michielbdejong commented Jan 6, 2018

Ah ok, then Transfer is a correct name. But if we go in that direction, then we should put Fulfill and Reject into Response packets, and we need to mark the three existing calls as deprecated, otherwise it will be very confusing. And this would be a breaking change, of which this PR by itself seems to describe just a small part, right?

A BTP connection/relationship no longer has a balance

I think a good way to explain the new situation is to avoid the term 'balance', and say the Message calls can increase your unsecuredAmount, while Transfer calls decrease it.

If you use BTP as just a trustline, then the unsecuredAmount plays the role of a "balance that shall not be named"; the reason we should not call it a balance is that it works in the opposite direction: unsecured amount goes up when you send Interledger payments, and it goes down when you receive them.

I think keeping the current BTP interpretation (where a Prepare+Fulfill decreases your balance instead of increasing your unsecuredAmount) would be much easier. We could just add a Settle call, which is the opposite of Transfer, and then we don't have to invert all this balance logic. But, up to you. :)

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

before merging this, we should discuss another option which @emschwartz came up with, which would be to just send paychan claim updates in Message calls (maybe using a protocolName that's not 'ilp', to distinguish it from Message calls that correspond to plugin.sendData calls).

I think that would make more sense, because there's no longer a balance to keep track of at the ledger layer, so no need for a separate Transfer call type at the BTP level.

And then we would be left with just an application-agnostic bilateral message/response/error protocol which is only called a 'transfer protocol' for historical reasons; if we would name it now, we would probably call it 'Bilateral Message Protocol' instead.

@michielbdejong
Copy link
Contributor

I would propose we keep putting the payment channel claim updates in Transfer calls for the 18Q1 version, since that's what we've already been implementing, and I want to put this live on the testnet today.

Then we can always move them to Message calls in the 18Q2 version.

@emschwartz
Copy link
Member

Maybe the real benefit of the Transfer type is so that we can clearly distinguish between the sendData and sendMoney methods in LPI2

@michielbdejong
Copy link
Contributor

clearly distinguish between the sendData and sendMoney methods in LPI2

The protocolName would be 'ilp' for sendData, and something else (e.g. 'paychan') for sendMoney.

@emschwartz
Copy link
Member

So the only difference when using the Transfer is that there is an amount field that you need to check against the claim that you've gotten to make sure they match? That sounds like it adds a potential footgun without any benefit, no?

@sharafian
Copy link
Author

Currently, it's used as an extra check to see if the balance is in dispute (the TRANSFER amount says how much the peer thinks this claim increases the balance). We could switch over to using MESSAGE without much trouble, though.

@emschwartz
Copy link
Member

What happens if they are out of sync? Would it just log a warning to the console? Doesn't seem like there's much you would actually do about that

@sharafian
Copy link
Author

Yeah, it just logs a warning to make you aware of it

@michielbdejong michielbdejong mentioned this pull request Jan 31, 2018
@michielbdejong
Copy link
Contributor

@sharafian I'm merging this so I can continue #383 on top of it, see further discussion there.

@michielbdejong michielbdejong merged commit 17d1535 into master Mar 5, 2018
@adrianhopebailie adrianhopebailie deleted the bs-btp-transfer branch May 24, 2018 21:28
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

4 participants