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

V0.2 #15

Closed
wants to merge 32 commits into from
Closed

V0.2 #15

wants to merge 32 commits into from

Conversation

expede
Copy link
Member

@expede expede commented Mar 24, 2023

Preview 📄

Changelog

  • Rename on to uri
  • Version with capsule types
  • Extract signature to a wrapper

@cla-bot cla-bot bot added the cla-signed label Mar 24, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@expede expede mentioned this pull request Mar 24, 2023
@expede
Copy link
Member Author

expede commented Mar 24, 2023

Progress report:

  1. Waiting for feedback on replacing Authorization with a varsig (happy to revert if that's faster/better)
  2. Need to update the examples (I'm partway through — not that much work, but it's getting late. Can probably have them fully done by EOD Friday)

Updated the schema and general layout of the doc. I think if we agree on the schema, we lock that in and I can wrap up cleaning up the prose etc to reflect the chanegs through Friday.

cc @Gozala & @zeeshanlakhani

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@zeeshanlakhani
Copy link

#15 (comment)

Yeah, I'm not so keen on that per se, as it can get problematic over time, but maybe @expede has thoughts on another approach for internals.

README.md Outdated Show resolved Hide resolved
README.md Outdated
meta {String : Any}
type Authorization struct {
scope [&Any] # The set of authorized links
auth Authorization # Scope signed by the invoker
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you didn't mean to change it.

Suggested change
auth Authorization # Scope signed by the invoker
s VarSig # Scope signed by the invoker

README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 556 to 558
type InvocationCapsule struct {
inv Invocation (rename "ucan/invoke@0.2.0")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type InvocationCapsule struct {
inv Invocation (rename "ucan/invoke@0.2.0")
}
type InvocationCapsule union {
| Invocation "ucan/invoke@0.2.0"
} representation keyed

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 248 to 251
type Receipt struct {
ocm Outcome
sig Varsig
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment...

ChainAgnostic/varsig#6 (comment)

...mentions this...

Screenshot 2023-03-29 at 23 23 18

...which is also my assumption about how varsig will work. If that gets merged, we could consider not having a Receipt wrapper here at all, since it would be redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was assuming that we sign Outcome and not the Receipt without sig.

expede and others added 4 commits March 29, 2023 23:24
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Signed-off-by: Brooklyn Zelenka <be.zelenka@gmail.com>
README.md Outdated
type Invocation struct {
v SemVer
```Task
type Context struct {

Choose a reason for hiding this comment

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

@expede should be type Task no? Task is in the markdown, but that's odd. Later we have https://github.com/ucan-wg/invocation/pull/15/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R207 which is the same?

README.md Outdated

## 5.2 (Signed) Invocation

An `Invocation` is a signed `Context`.

Choose a reason for hiding this comment

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

minor: we don't have Context as a thing in the updated doc :).


# All the other metadata
meta {String : Any}
meta {String : Any} # All the other metadata

# Principal that issued this receipt. If omitted issuer is

Choose a reason for hiding this comment

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

going off the Ucan spec, is this not Principle?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that would be a typo iun the UCAN spec then 😅 For example, from the JWT spec:

The "iss" (issuer) claim identifies the principal that issued the JWT.

Copy link
Member Author

@expede expede Apr 7, 2023

Choose a reason for hiding this comment

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

The UCAN spec actually uses both senses of principle/principal, which can be confusing:

Following the principle of least authority

...and...

6.2 Principal Alignment

Choose a reason for hiding this comment

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

ah yes. rs-ucan uses Principle (in the code) haha.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh LOL we should probably fix that

@expede
Copy link
Member Author

expede commented Aug 21, 2023

A bunch of this is moving to https://github.com/ucan-wg/invocation-ipld/pull/1

FYI: I'm forking this branch to do the high level invocation spec

@Gozala
Copy link
Contributor

Gozala commented Sep 28, 2023

Where are we with this, can this be merged ? We'd like to ship invocation spec but there's quite bit a difference between what's in the main branch and here. If we moving to this it would really help to have thing merged or at least field renames so they aren't completely different.

@expede
Copy link
Member Author

expede commented Oct 16, 2023

Closing in favour of #21

@expede expede closed this Oct 16, 2023
@expede expede deleted the v0.2 branch October 16, 2023 05:22
@Gozala Gozala restored the v0.2 branch October 18, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants