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

STREAM RFC: codify the conditions for a ConnectionAssetDetails frame being returned from a receiver. #546

Closed
theotherian opened this issue Sep 25, 2019 · 3 comments · Fixed by #551

Comments

@theotherian
Copy link

theotherian commented Sep 25, 2019

Information that inspired this issue:

Proposal:
STREAM RFC (il-rfc29) should codify the conditions for a ConnectionAssetDetails frame being returned from a receiver.

Question: What is the correct way to indicate a new Connection for a receiver?
Option1: Treat sequence of 1 as a new connection.
Option2: Whenever a ConnectionNewAddressFrame and a SendMoneyFrame are encountered.
Option3: Leave the RFC as is, which suggests only stateful receivers (see next question).

Question: should there be stateless receivers, or is this a protocol violation?

Question: What should a receiver reply with on a new connection?
Stateless:

  • When receiver receives a ConnectionNewAddress and a StreamMoneyFrame frame from a sender, it should reply with a ConnectionAssetDetails frame.

Stateful:

  • When receiver receives its first StreamMoneyFrame for a given connection, it should reply with a ConnectionAssetDetails frame.
@theotherian theotherian changed the title STREAM RFC: When a receiver (stateless or stateful) detects a "new connection", it MUST return at least a ConnectionAssetDetailsFrame. STREAM RFC: codify the conditions for a ConnectionAssetDetails frame being returned from a receiver. Sep 25, 2019
sappenin added a commit that referenced this issue Oct 8, 2019
* Update STREAM RFC to clarify how `ConnectionAssetDetails` are communicated for a given STREAM Connection.
* Clarify the relationship between `ConnectionAssetDetails` and `ConnectionNewAddress `.

Signed-off-by: sappenin <[email protected]>
@sappenin sappenin mentioned this issue Oct 8, 2019
@sappenin
Copy link
Contributor

sappenin commented Oct 8, 2019

After discussing with @theotherian and @nhartner, we think the following are the correct path forward (also represented by PR #551):

Question: What is the correct way to indicate a new Connection for a receiver?

We want the RFC to be able to support stateless receivers and bi-directional streams (not at the same time, but in general -- and maybe only in the future). To this end, we shouldn't specify returning ConnectionAssetDetails on the first stream packet sequence number, because this may not be the point in time that one side or the other requires this information.

Instead, we should maintain the current Javascript Connector functionality (which is also what the Java Stream receiver does) and simply mandate that any endpoint receiving ConnectionNewAddress should respond with ConnectionAssetDetails. See the RFC PR (#551) for exact semantics here.

Question: should there be stateless receivers.

Yes, there are already stateless receivers in the wild that work just fine, so the RFC should not preclude them.

Question: What should a receiver reply with on a new connection?

See #551.

@adrianhopebailie
Copy link
Collaborator

@sappenin and @kincaidoneil interledgerjs/ilp-protocol-stream#112 was never resolved, does it have any bearing on this?

@kincaidoneil
Copy link
Member

was never resolved, does it have any bearing on this?

Not really, I think it just underscores that it's important for the sender and recipient to exchange asset details, so we should support that for stateless receivers. AFAIK, David's PR should be fully compatible with the existing JS Stream implementation.

adrianhopebailie pushed a commit that referenced this issue Oct 23, 2019
* Fixes #546

* Update STREAM RFC to clarify how `ConnectionAssetDetails` are communicated for a given STREAM Connection.
* Clarify the relationship between `ConnectionAssetDetails` and `ConnectionNewAddress `.

Signed-off-by: sappenin <[email protected]>

* Update 0029-stream/0029-stream.md

Co-Authored-By: Kincaid O'Neil <[email protected]>

* Update per PR comments.

Signed-off-by: sappenin <[email protected]>

* Remove redundant clarification.

Signed-off-by: sappenin <[email protected]>

* Remove wordy sentence

Signed-off-by: sappenin <[email protected]>

* Remove language about ignoring subsequent frames.

Signed-off-by: sappenin <[email protected]>

* Removing breaking proposed changes.

Signed-off-by: sappenin <[email protected]>

* Remove whitespace

Signed-off-by: sappenin <[email protected]>

* Remove inaccurate example.

Signed-off-by: sappenin <[email protected]>

* Bumping draft number to test the build scripts

Signed-off-by: sappenin <[email protected]>

* Reducing the draft num

Signed-off-by: sappenin <[email protected]>

* Restoring correct draft number

Signed-off-by: sappenin <[email protected]>

* Skip Version Check

Skip Version Check

Signed-off-by: sappenin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants