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

BTP/2.0 #383

Merged
merged 4 commits into from
Mar 7, 2018
Merged

BTP/2.0 #383

merged 4 commits into from
Mar 7, 2018

Conversation

michielbdejong
Copy link
Contributor

This PR continues the work started in #360, removing the deprecated parts from BTP, and removing some talk about ledgers which no longer applies now that we talk about sending "data" rather than conditional transfers.

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.

Great stuff, thanks for updating this spec!

| 3 | `Prepare` | Request |
| 4 | `Fulfill` | Request |
| 5 | `Reject` | Request |
| 3 | (deprecated) | |
Copy link
Member

Choose a reason for hiding this comment

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

To me "deprecated" means "may be removed in the future". If you don't describe those messages at all anymore I would assume that new implementations don't support them at all, so something like "(removed)" or "(outdated)" may be better.

their balance. If Peer 1 owes Peer 2 100 XRP, Peer 1 could send Peer 2 100
XRP on Ripple in order to make their balance 0. In that scenario, XRP ledger is
the underlying ledger.
has been sent yet.
Copy link
Member

Choose a reason for hiding this comment

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

Should this be "no response has been received yet."?

who is running your plugin.
When two Interledger connectors use [OER in HTTP POST bodies](...) as the communication channel between
them, they each need to act as a HTTP server at times. If one of the connectors runs behind a firewall,
this may be impossible. Therefore, we foresee in the option of using WebSockets instead of HTTP. With
Copy link
Member

Choose a reason for hiding this comment

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

"in the option"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry, Dutchism "voorzien in" :)


Data to secure this payment (such as payment channel claims), can be attached
under the protocol data.
`Transfer` is used to send payment channel claims to the other connector.
Copy link
Member

Choose a reason for hiding this comment

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

True, but I wonder if this is too specific. Could Transfers be used to send - for instance - a PayPal payment ID as well to trigger processing of some other form of outside settlement?

@michielbdejong michielbdejong dismissed justmoon’s stale review March 5, 2018 12:36

changes addressed, thanks!

@michielbdejong michielbdejong changed the base branch from bs-btp-transfer to master March 5, 2018 15:23
Copy link
Collaborator

@adrianhopebailie adrianhopebailie left a comment

Choose a reason for hiding this comment

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

Some editorial stuff and a question around the correct definition of Transfer.amount

These allow you to send interledger payments through anyone else on that ledger
who is running your plugin.
When two Interledger connectors send ILPv4 packets over HTTP POST,
they each need to act as a HTTP server at times. If one of the connectors runs behind a firewall,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"as an HTTP server"

@@ -232,7 +209,7 @@ For example, the primary sub-protocol entry of a Message might represent a quote

Likewise, only the primary sub-protocol data in a Response indicates whether result of the request from the Message being responded to actually succeeded or not.

In Error, Prepare, Fulfill, and Reject calls, the distinction between primary and secondary sub-protocol entries is less strict.
In Error calls, the distinction between primary and secondary sub-protocol entries is less strict.

## Flow

Copy link
Collaborator

Choose a reason for hiding this comment

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

"The request types are Message, Prepare, Fulfill and Reject" should be "The
request types are Message and Transfer"

@@ -282,140 +259,6 @@ protocol data.
not include ILP errors such as "No quote found", only the cases in which the
Copy link
Collaborator

Choose a reason for hiding this comment

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

"ILQP packets" should be "ILP packets"

`Transfer` is used to send proof of payment, payment channel claims, or other
settlement information to the other connector.
The amount should indicate the relative value of this settlement state (compared to the
previous settlement state), in a unit that was agreed out-of-band.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Relative means this can be negative.

Is this description correct?

I thought this was used to carry a sendMoney which means the amount is always positive and the message is only sent by the connector that is making the transfer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I changed "relative" to "additional"

@michielbdejong michielbdejong dismissed adrianhopebailie’s stale review March 7, 2018 09:13

all comments address, thanks a lot!

@michielbdejong michielbdejong merged commit af1acf2 into master Mar 7, 2018
@michielbdejong michielbdejong deleted the mj-btp-2 branch March 7, 2018 09:16
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