-
Notifications
You must be signed in to change notification settings - Fork 280
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
p2p-circuit v2 spec #325
p2p-circuit v2 spec #325
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 Thanks @vyzo for creating the draft!
optional int64 data = 2; // bytes | ||
} | ||
|
||
enum Status { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to split Status
in HopStatus
and StopStatus
, making invalid states impossible at the type level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's type pollution and these invalidities are very much trivial; I don't feel too strongly about it however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, implementations must be prepared to handle unknown error status codes anyway, to support future extensions.
Fleshed out the remaining sections and turned the thorny integers into unsigned. @mxinden this should be ready for review; note the changes in the vouchers, they are now signed envelopes (which means both our implementations will need to change). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fleshed out the remaining sections
Thank you for the detailed protocol description. Very much appreciated.
For the sake of consistency across the specifications use plantuml to picture circuit relay v2 protocol interaction.
relay/circuit-v2: Replace excalidraw with plantuml
summoning @Stebalien @raulk -- this is ready for your review. |
@mxinden any concerns with landing this? The spec is still working draft so edits can still be subsequently pushed as needed until we hit Candidate Recommendation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mxinden any concerns with landing this? The spec is still working draft so edits can still be subsequently pushed as needed until we hit Candidate Recommendation.
Merging and addressing all future changes through separate pull requests sounds good to me.
In general I am in favor of many small pull requests over one large pull request, as a single large pull request is at risk of being stalled due to a couple small discussion points.
@vyzo friendly ping. Can this be merged? |
needs a couple more edits, will get to it.
…On Mon, Jun 7, 2021, 17:22 Max Inden ***@***.***> wrote:
@vyzo <https://github.com/vyzo> friendly ping. Can this be merged?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#325 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAI4SVRCGKD7M6XWBH55E3TRTI35ANCNFSM44EPM74Q>
.
|
@raulk this should be ready for merge. |
no objections from me, waiting on @raulk to green tick. |
No description provided.