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

Address additional feedback to CapTP spec #60

Merged
merged 6 commits into from
Jan 24, 2024
Merged

Conversation

tsyesika
Copy link
Contributor

I had missed some of the feedback Richard Gibson had given in review on the CapTP specification and thought it was good to merge. This PR contains the changes I've made to address that feedback.

The feedback itself is on #42

I don't suspect this PR needs to take too long for reviewing as really it's part of #42.

@dckc
Copy link
Collaborator

dckc commented May 26, 2023

@gibson042 please give this a look.

@tsyesika @jar398 I can't seem to request review from @gibson042 in the normal github way. Would you please grant @gibson042 whatever permission would fix that?

@jar398
Copy link
Contributor

jar398 commented May 26, 2023

@dckc : @gibson042 access granted

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. Other suggestions that could be applied (but can't be represented with inline comments because they're too far from changes):

#42 (comment)

@@ -18,3 +18,3 @@
-functionality without compromise. In this model, an object can use a reference
-to another object (capability) if and only if it possesses a reference to it.
+functionality without compromise. In this model, an object can use another
+object (capability) if and only if it possesses a reference to it.
 In other words, "if you don't have it, you can't use it."

#42 (comment)

@@ -76,2 +76,2 @@
     -   Both parties cooperate to free object references which are recognized to
-        no longer be needed to be imported from one side.
+        no longer be needed on at least one side.

#42 (comment)

@@ -121,2 +121,2 @@
 message, and the remaining arguments are the message sent to that behavior. This
-convention is followed for the CapTP bootstrap object & promises.
+convention is followed for the CapTP bootstrap object and for promises.

#42 (comment)

@@ -124,6 +124,6 @@
 ## Ending a session
 
-Finally, a session may end due to an error occuring that could not be continued
-from or because either side wishes to end the session. Both situations are
-covered by the [`op:abort`](#op-abort) message. When this is received the
-session should be severed and unresolved promises broken.
+Finally, a session may end due to an unhandled/unrecoverable error or because
+either side wishes to end it. Both situations are covered by the
+[`op:abort`](#op-abort) message. When this is received the session should be
+severed and unresolved promises broken.

#42 (comment)
What part of the first paragraph under Promise and Resolver Objects explains "come as a pair"?

#42 (comment)
This document should be consistent w.r.t. promise vocabulary. Is "unresolved" ∪ "fulfilled" ∪ "broken" a complete characterization?

#42 (comment)

@@ -146,3 +146,3 @@
 "promise" object which represents a value and may eventually resolve to either a
-value or object reference or may break, either explicitly, implicitly through
-error handling, or via network partition.
+value or object reference, or may break by either explicit instruction or by
+implicit error propagation or network partition.

#42 (comment)

@@ -152,2 +152,2 @@
 There is also a "promise resolver" object which is used to provide the promise a
-value or break on an error. The resolver object has two methods:
+fulfillment value or break reason. The resolver object has two methods:

#42 (comment) requests explanation for the asymmetry of fulfill accepting multiple arguments but break accepting only one.

#42 (comment)

@@ -438,4 +438,4 @@
 The bootstrap object is responsible for providing access to local objects on the
-session. It has two different behaviors, these are selected using the
-conventional CapTP method mechanism of sending a symbol as the first argument,
-the following methods are available:
+session. It has three different behaviors, selected using the conventional CapTP
+method mechanism of sending a symbol as the first argument.
+The following methods are available:

#42 (comment)

@@ -478,4 +478,4 @@
 <op:deliver-only <desc:export 0>                               ; Remote bootstrap object
-                  ['deposit-gift                               ; Argument 1: Symbol "deposit-gift"
+                 ['deposit-gift                                ; Argument 1: Symbol "deposit-gift"
                   gift-id                                      ; Argument 2: Positive integer or 0
-                  <desc:import-object | desc:import-promise>)> ; Argument 3
+                  <desc:import-object | desc:import-promise>]> ; Argument 3

#42 (comment)

@@ -487,1 +487,1 @@
-in order to receive a gift. It has one arguments:
+in order to receive a gift. It has one argument:

#42 (comment)

@@ -497,5 +497,5 @@
 <op:deliver <desc:export 0>           ; Remote bootstrap object
             [withdraw-gift            ; Argument 1: Symbol "withdraw-gift"
              <desc:handoff-receive>]  ; Argument 2: desc:handoff-receive
-            1                         ; Positive integer
+            1                         ; Answer position: positive integer
             <desc:import-object 3>>   ; The object exported (by us) at position 3, should receive the gift.

#42 (comment) asks why answer-pos values in the various messages differ (positive integer or 0 for op:bootstrap, positive integer or false for op:deliver, positive integer for op:pick and op:gc-answer).

#42 (comment)

@@ -568,4 +568,4 @@
 This answer position is then referenced with a [`desc:answer`](#desc-answer)
-descriptor. Note that when the answer position is longer needed, it's important
-to notify the other side with a [`op:gc-answer`](#op-gc-answer) message (see
-section for details).
+descriptor. Note that when the answer position is no longer needed, it's
+important to notify the other side with a [`op:gc-answer`](#op-gc-answer)
+message (see section for details).

#42 (comment)

@@ -589,1 +589,1 @@
-Messages send to this promise MUST be delivered to the object when the promise
+Messages sent to this promise MUST be delivered to the object when the promise

#42 (comment) requests a section dedicated to describing "answer position" vs. at least "export position", rather than just a small aside buried in op:deliver.

#42 (comment)

@@ -601,3 +601,3 @@
 exported at a specific answer position. If the promise represents a single
-value, only a `selected-value-pos` of `0` is valid, of course sending a message
-to that single value without an `op:pick` is also perfectly valid.
+value, only a `selected-value-pos` of `0` is valid, but in that case the pick
+is itself unnecessary because messages could already be sent to that promise.

#42 (comment)

@@ -607,3 +607,3 @@
 <op:pick  [<promise-pos>         ; <desc:answer | desc:import-promise>
-           <selected-value-pos>  ; Positive Integer
+           <selected-value-pos>  ; Positive Integer or 0
            <new-answer-pos>]>    ; Positive Integer

#42 (comment)

@@ -656,8 +656,8 @@
-The `wants-partial?` flag indicates if a "partial" update should be provided as
-the notification when the promise resolves. If `wants-partial?` is false and the
-promise resolves to another promise, a notification to the `listen-desc` is not
-sent. A notification is only ever sent to the `listen-desc` when the promise
-resolves to a non-promise value, or breaks. If `wants-partial?` is true, a
-notification on any resolution, including a promise. In the case it does notify
-on a promise the listen is considered complete, if future notifications are
-desired, further `op:listen` operations should be sent to the promises.
+The `wants-partial` flag indicates if a "partial" update should be provided as
+the notification to `listen-desc` when the promise resolves to another promise.
+If `wants-partial` is false, a notification is sent only when the promise
+resolves to a non-promise value or breaks (i.e., not when it resolves to another
+promise). If `wants-partial` is true, a notification is sent for any resolution,
+including to another promise. Any notification is considered to conclude the
+`op:listen` interaction, and if future notifications are desired (e.g., after a
+partial notification) then further `op:listen` operations should be sent.

draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
This value is actually the location wrapped in a Syrup record that has
the label `my-location`. This has been done to prevent this signature
being used in other contexts.
@tsyesika
Copy link
Contributor Author

tsyesika commented Sep 7, 2023

This should be ready for review and hopefully if the group agrees being merged. I think I've addressed all you feedback @gibson042. There were a few pieces of feedback on the promises section however I re-wrote that a bit to make some things clearer based on feedback.

The other thing this includes is e4e7fe9 which I added based on this issue on the test suite ocapn/ocapn-test-suite#3

@jar398 jar398 mentioned this pull request Sep 12, 2023

The promise object may eventually resolve to either a concrete value, object
reference, another promise (in the case of promise pipelining), or may break.
When a promise breaks its resolved with an error, breakages can be caused by
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
When a promise breaks its resolved with an error, breakages can be caused by
When a promise breaks it is resolved with an error. Breakages can be caused by

The promise object may eventually resolve to either a concrete value, object
reference, another promise (in the case of promise pipelining), or may break.
When a promise breaks its resolved with an error, breakages can be caused by
either explicit instruction, by implicit error propagation, or network
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
either explicit instruction, by implicit error propagation, or network
either explicit instruction, by implicit error propagation, or by network

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Some comments may still be unresolved:

@tsyesika
Copy link
Contributor Author

Some comments may still be unresolved:

* [Add draft CapTP spec #42 (comment)](https://github.com/ocapn/ocapn/pull/42#discussion_r1175962650)
  This document should be consistent w.r.t. promise vocabulary. Is "unresolved" ∪ "fulfilled" ∪ "broken" a complete characterization?

I think so in that:

unresolved: The promise has no resolution, that is that it is not fulfilled, nor resolved.
fulfilled: The promise has been fulfilled with one or more values
broken: The promise has been broken (break) with an error.

I did have a look though and thought the usage of those terms seemed to be correct, but I may have missed some so if you've noticed any specific cases, I can fix those.

* [Add draft CapTP spec #42 (comment)](https://github.com/ocapn/ocapn/pull/42#discussion_r1175965738) requests explanation for the asymmetry of `fulfill` accepting multiple arguments but `break` accepting only one.

* [Add draft CapTP spec #42 (comment)](https://github.com/ocapn/ocapn/pull/42#discussion_r1201176140) asks why `answer-pos` values in the various messages differ (positive integer or 0 for op:bootstrap, positive integer or false for op:deliver, positive integer for op:pick and op:gc-answer).


* [Add draft CapTP spec #42 (comment)](https://github.com/ocapn/ocapn/pull/42#discussion_r1201198686) requests a section dedicated to describing "answer position" vs. at least "export position", rather than just a small aside buried in op:deliver.

* [Add draft CapTP spec #42 (comment)](https://github.com/ocapn/ocapn/pull/42#discussion_r1201204650) requests a caveat explaining a situation in which pick is unnecessary.

* [Add draft CapTP spec #42 (comment)](https://github.com/ocapn/ocapn/pull/42#discussion_r1201199451) requests explicit inclusion of 0 alongside "Positive Integer".

The first one I think I've addressed so left some notes there, the rest however probably could use some further work. Since this is quite a large change and it's already been approved in the September 2023 meeting. I wonder if we should merge this PR as is and I will make a follow up PR before the next meeting (hopefully with ample time for you to review it) with the rest. WDYT @gibson042 ?

@zarutian
Copy link

zarutian commented Jan 9, 2024

I am for this merge.

Copy link
Collaborator

@gibson042 gibson042 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 suggestions, but nothing serious AFAICT.

I wonder if we should merge this PR as is and I will make a follow up PR [for #60 (review)]

Works for me.

@@ -74,29 +74,26 @@ Here's an overview of some things which may occur during a CapTP session:
- Handoffs are initiated when sending a message with a reference to an
object outside of the CapTP session.
- Both parties cooperate to free object references which are recognized to
no longer be needed to be imported from one side.
no longer be needed on at least one side
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible improvement:

Suggested change
no longer be needed on at least one side
have become {unnecessary,irrelevant,unreachable,…} on at least one side

maintaining the security CapTP provides, we need to perform a [Third Party
Handoff](#third-party-handoffs).
maintaining the security provided by CapTP, it is necessary to perform a
[Third Party Handoff](#third-party-handoffs).

It is conventional, but not required, that some objects may offer different
behavior depending on a "method". The method is conventionally a symbol whose
value is the name of the method/behavior. This symbol is the first argument in a
message, and the remaining arguments are the message sent to that behavior. This
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand what it means to send a message to a behavior.

Suggested change
message, and the remaining arguments are the message sent to that behavior. This
message, and the remaining arguments are sent to that behavior. This


The promise object may eventually resolve to either a concrete value, object
reference, another promise (in the case of promise pipelining), or may break.
When a promise breaks its resolved with an error, breakages can be caused by
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
When a promise breaks its resolved with an error, breakages can be caused by
When a promise breaks it is resolved with an error. Breakages can be caused by

The promise object may eventually resolve to either a concrete value, object
reference, another promise (in the case of promise pipelining), or may break.
When a promise breaks its resolved with an error, breakages can be caused by
either explicit instruction, by implicit error propagation, or network
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
either explicit instruction, by implicit error propagation, or network
either explicit instruction, by implicit error propagation, or by network

There is also a "promise resolver" object which is used to provide the promise a
value or break on an error. The resolver object has two methods:
Promises can be listened to with the [`op:listen`](#op-listen) operation, or
messages can be sent to them as if it were the resolved object. The messages
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
messages can be sent to them as if it were the resolved object. The messages
messages can be sent to a promise as if it were its eventually resolved object. The messages

Everything except the `...` must be verbatim. The `...` is replaced by Binary
Data typed values representing the public key.
In the above format, each `(` and `)` mark the start and end of a new Syrup
sequence with each value with the sequence being a symbol except the `...`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sequence with each value with the sequence being a symbol except the `...`,
sequence with each value in the sequence being a symbol except the `...`,

Everything except the `...` must be verbatim. The `...` is replaced by Binary
Data typed values representing the signature.
In the above format, each `(` and `)` mark the start and end of a new Syrup
sequence with each value with the sequence being a symbol except the `...`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sequence with each value with the sequence being a symbol except the `...`,
sequence with each value in the sequence being a symbol except the `...`,

Comment on lines +317 to +318
3. Replace the reference to the remote object being gifted in the message to be
the signed handoff give in step 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
3. Replace the reference to the remote object being gifted in the message to be
the signed handoff give in step 2
3. Replacing the reference to the remote object being gifted in the message with
signed handoff from step 2

per-session key pair. This is serialized in accordance with
[Cryptography](#cryptography)
The `acceptable-location-sig` is a cryptographic signature of
`acceptable-location`, wrapped with Syrup record with the label `my-location`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`acceptable-location`, wrapped with Syrup record with the label `my-location`.
`acceptable-location`, wrapped with a Syrup record with the label `my-location`.


### Receiving

The `captp-version` MUST be equal to `1.0`. If the version does not match, the
connection MUST be aborted.

The `acceptable-location-sig` MUST be valid that the `session-pubkey` provided a
valid signature of `acceptable-location`.
valid signature of `acceptable-location` wrapped with the Syrup record with the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
valid signature of `acceptable-location` wrapped with the Syrup record with the
valid signature of `acceptable-location` wrapped with a Syrup record with the

Copy link
Collaborator

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

In my new role as the Agoric Proconsul Editor, I deem these changes to be editorial in nature and to not require normative feedback at our regular meetings.

@tsyesika tsyesika merged commit 90ab732 into main Jan 24, 2024
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.

6 participants