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

Fixes #546 #551

Merged

adrianhopebailie merged 14 commits into master from df/clarify-stream-connection-details

Oct 23, 2019

Contributor

sappenin commented Oct 8, 2019 •

edited

Loading

Fixes #546 by clarifying the STREAM RFC.

Clarify that ConnectionAssetDetails MUST not change during the lifetime of a Connection.

Signed-off-by: sappenin sappenin@gmail.com


          Fixes #546

81ef118

* 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 <sappenin@gmail.com>

sappenin requested review from adrianhopebailie, justmoon, sentientwaffle, dora-gt, emschwartz, sharafian, kincaidoneil, matdehaast and gakonst

October 8, 2019 19:22

sappenin mentioned this pull request

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

Closed

kincaidoneil reviewed

View reviewed changes

0029-stream/0029-stream.md Outdated Show resolved Hide resolved

0029-stream/0029-stream.md Outdated Show resolved Hide resolved

sappenin and others added 2 commits

October 8, 2019 17:04


          Update 0029-stream/0029-stream.md

32be872

Co-Authored-By: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>


          Update per PR comments.

Signed-off-by: sappenin <sappenin@gmail.com>

sappenin commented

View reviewed changes

0029-stream/0029-stream.md Outdated Show resolved Hide resolved


          Remove redundant clarification.

5c87622

Signed-off-by: sappenin <sappenin@gmail.com>

kincaidoneil requested a review from traviscrist

October 9, 2019 03:24

sappenin mentioned this pull request

Tracking: RFC Breaking Changes for STREAM V2 #554

Closed

sappenin added 3 commits

October 9, 2019 08:49


          Merge branch 'master' of github.com:interledger/rfcs into df/clarify-…

575cab2

…stream-connection-details


          Remove wordy sentence

fcb0dcb

Signed-off-by: sappenin <sappenin@gmail.com>


          Remove language about ignoring subsequent frames.

c61fb33

Signed-off-by: sappenin <sappenin@gmail.com>

emschwartz requested changes

View reviewed changes

Member

emschwartz left a comment

I don't think we should make any changes to the spec until we've implemented said changes and have some use case that requires this to be done the same way in all implementations. This frame is already part of the protocol so we can use it for now. In the future we may choose to deprecate it or make it mandatory but I don't think we have enough information to make that determination now.


          Removing breaking proposed changes.

a1b9ea8

Signed-off-by: sappenin <sappenin@gmail.com>

Contributor Author

sappenin commented Oct 9, 2019 •

edited

Loading

I don't think we should make any changes to the spec until we've implemented said changes and have some use case that requires this to be done the same way in all implementations.

Sounds fine to me. I have removed any breaking changes from this PR - the only thing left is non-breaking clarifications to the spec.

In the Java STREAM implementation, we're going to support what JS is doing (and apparently what Rust is going to do). We might experiment with what was proposed earlier in this PR (i.e., respond to a ConnectionAssetDetails frame with a corresponding ConnectionNewAddress frame) and see if it adds any value.

As a community, we can revisit the desired behavior here sometime in the future if we ever decide this is worth changing (see #554, which is just a tracking issue, if you're interested).

sappenin requested review from emschwartz and kincaidoneil

October 9, 2019 20:04


          Remove whitespace

24aeec6

Signed-off-by: sappenin <sappenin@gmail.com>

emschwartz approved these changes

View reviewed changes

sharafian approved these changes

View reviewed changes

sharafian left a comment

One comment which is unrelated to this change but could be added in as another quick fix

0029-stream/0029-stream.md Outdated Show resolved Hide resolved

gakonst approved these changes

View reviewed changes


          Remove inaccurate example.

eb5cc17

Signed-off-by: sappenin <sappenin@gmail.com>

sappenin added the non-breaking label

sappenin added this to the Protocol Specs milestone

theotherian approved these changes

View reviewed changes

sappenin assigned adrianhopebailie

Contributor Author

sappenin commented Oct 17, 2019

@adrianhopebailie The build doesn't seem to be working for whatever reason. Once you determine what the problem is, let me know and I can push another change.

Collaborator

adrianhopebailie commented Oct 17, 2019

@mDuo13 @ryangyoung any idea what could be happening here. Could this be related to the change of template?

sappenin added 4 commits

October 23, 2019 08:52


          Bumping draft number to test the build scripts

6d4af7c

Signed-off-by: sappenin <sappenin@gmail.com>


          Reducing the draft num

b6fa521

Signed-off-by: sappenin <sappenin@gmail.com>


          Restoring correct draft number

cdb0085

Signed-off-by: sappenin <sappenin@gmail.com>


          Skip Version Check

f1e4dc2

Skip Version Check

Signed-off-by: sappenin <sappenin@gmail.com>

adrianhopebailie merged commit 734c2f0 into master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

theotherian theotherian approved these changes

emschwartz emschwartz approved these changes

sharafian sharafian approved these changes

gakonst gakonst approved these changes

adrianhopebailie Awaiting requested review from adrianhopebailie

justmoon Awaiting requested review from justmoon

sentientwaffle Awaiting requested review from sentientwaffle

dora-gt Awaiting requested review from dora-gt

matdehaast Awaiting requested review from matdehaast

traviscrist Awaiting requested review from traviscrist

kincaidoneil Awaiting requested review from kincaidoneil