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

Decouple CAIP-27 from CAIP-25 and introduce CAIP-217 for Authorization Scopes #217

Merged
merged 32 commits into from
May 19, 2023

Conversation

bumblefudge
Copy link
Collaborator

@bumblefudge bumblefudge commented Mar 2, 2023

To-do before merge

@hmalik88 @pedrouid please opine on the questions above and I can throw some more commits in this bad boy

CAIPs/caip-25.md Outdated Show resolved Hide resolved
CAIPs/caip-27.md Outdated Show resolved Hide resolved
CAIPs/caip-27.md Outdated Show resolved Hide resolved
CAIPs/caip-27.md Outdated Show resolved Hide resolved
bumblefudge and others added 2 commits March 3, 2023 18:12
Co-authored-by: Hassan Malik <[email protected]>
Co-authored-by: Hassan Malik <[email protected]>
Copy link
Collaborator

@hmalik88 hmalik88 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator Author

@bumblefudge bumblefudge left a comment

Choose a reason for hiding this comment

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

LGTM, @pedrouid final check?

@bumblefudge bumblefudge changed the title Update CAIP-27 to be constrained by CAIP-25 session authZ Decouple CAIP-27 from CAIP-25 and introduce CAIP-217 for Authorization Scopes Mar 29, 2023
@bumblefudge
Copy link
Collaborator Author

Majorly refactored; new initial comment above

CAIPs/caip-27.md Outdated Show resolved Hide resolved
CAIPs/caip-27.md Outdated Show resolved Hide resolved
Copy link
Member

@pedrouid pedrouid left a comment

Choose a reason for hiding this comment

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

Just left a single comment to replace in caip_request with provider_request for CAIP-27

Other than that... This is looking amazing! 🙏

I can feel CAIP-25 and CAIP-27 being Final very very soon 🚀

CAIPs/caip-217.md Outdated Show resolved Hide resolved
CAIPs/caip-27.md Outdated Show resolved Hide resolved
CAIPs/caip-27.md Outdated Show resolved Hide resolved
@pedrouid
Copy link
Member

pedrouid commented Apr 5, 2023

Some extra comments to fix how markdown is being rendered plus making it more consistent with CAIP-25 naming

bumblefudge and others added 5 commits April 5, 2023 17:31
Co-authored-by: Pedro Gomes <[email protected]>
Co-authored-by: Pedro Gomes <[email protected]>
Co-authored-by: Pedro Gomes <[email protected]>
Co-authored-by: Pedro Gomes <[email protected]>
Co-authored-by: Pedro Gomes <[email protected]>
Copy link
Member

@pedrouid pedrouid left a comment

Choose a reason for hiding this comment

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

👍

CAIPs/caip-211.md Outdated Show resolved Hide resolved
Comment on lines 91 to 117
```jsonc
{
"X": {
methods: [A, B, C],
notifications: [D, E, F],
rpcEndpoints: [U1, U2, U3],
rpcDocuments: [V]
}
}
```
MUST be taken literally, as dropping the implicit values rather than appending
to them. It MUST NOT be interpreted as equivalent to:

```jsonc
{
"X": {
methods: [A, B, C],
notifications: [D, E, F],
rpcEndpoints: [Y1, Y2, Y3, U1, U2, U3],
rpcDocuments: [Z, V]
}
}
```

But the latter SHOULD be returned as response if dropping the implicit values in
favor of the requested ones is unacceptable and extending the implicit values is
preferred.
Copy link
Collaborator

@hmalik88 hmalik88 Apr 13, 2023

Choose a reason for hiding this comment

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

I think we need to think about this a bit more.

Points I agree with:

  1. You're basically saying that the first format should be used in a request and the second should be used in a response. Reason being because the rpcDocuments in the request could potentially overwrite the implicit values for that chain so it makes sense to drop implicit values entirely.

Points that are unclear:

  1. If the wallet/user-agent detects that the rpcDocuments are not overwriting implicit values it would include the rpc document defined in the CAIP profile for that chain which I think falls under the statement "if dropping the implicit values in favor of the requested ones is unacceptable". This is unclear to me, because what do we mean by unacceptable? Is unacceptable that the wallet isn't able to or doesn't want to match the function signature that the dapp is requesting for a particular method? In that case it should just reject the request. OR are we talking about a scenario where the request had methods in its methods array that the wallet had deemed to be implicit values?

  2. I'm also wondering that at the point a chain has a CAIP profile with defined rpc documents, the implicit rpc document would become extraneous information in the response. Do we just have dapps include what we consider to be "implicit" values in their own rpc document endpoints until a chain has it's own CAIP profile? How do we determine when that is? This seems like a very gray area.

    a. One thing to note is that we also don't want to return rpc documents for methods that haven't been requested. What happens in the likely scenario that a dapp is requesting a subset of a chain's implicit methods? Do we return the whole rpc document regardless?

Copy link
Collaborator Author

@bumblefudge bumblefudge Apr 13, 2023

Choose a reason for hiding this comment

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

Unclear # 1: by unacceptable, I meant "allowed to extend e.g. execution APIs but I neither want to allow your documents to contradict/re-define execution APIs NOR do I want to manually confirm each connection that your documents don't contradict/redefine. I can either make this more explicit in another commit... or change course if it's a faulty use-case assumption!

Unclear # 2: it's extraneous if it's in every request, in the sense that it verbosely states the obvious, but I wouldn't drop it from my responses to those verbose requests if I was a provider! what a CAIP-211 profile does is give additional confidence to the providers who want to interpret no setting as equivalent to execution-apis, and let them respond without adding it. I guess I was assuming legacy verbosity would be tolerable and just peter out over time harmlessly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unclear # a: I was assuming whole RPC document -- if machine-readable, "subsetting" that document or extracting, say, ABI signatures for function names can happen automatically, if human-readable (a permalink to a namespace's official dev docs, for example), this might be a little more onerous...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unclear # 1: by unacceptable, I meant "allowed to extend e.g. execution APIs but I neither want to allow your documents to contradict/re-define execution APIs NOR do I want to manually confirm each connection that your documents don't contradict/redefine. I can either make this more explicit in another commit... or change course if it's a faulty use-case assumption!

This should just be a rejected request then.

Unclear # 2: it's extraneous if it's in every request, in the sense that it verbosely states the obvious, but I wouldn't drop it from my responses to those verbose requests if I was a provider! what a CAIP-211 profile does is give additional confidence to the providers who want to interpret no setting as equivalent to execution-apis, and let them respond without adding it. I guess I was assuming legacy verbosity would be tolerable and just peter out over time harmlessly?

Ok that's fair. In the scenario that a dapp is requesting some implicit methods, it should also then provide documents for those methods until as you said legacy verbosity peters out over time. We would also then naturally return said documents in the response to maintain this statefulness through the chain of events idea we spoke about.

unclear # a: I was assuming whole RPC document -- if machine-readable, "subsetting" that document or extracting, say, ABI signatures for function names can happen automatically, if human-readable (a permalink to a namespace's official dev docs, for example), this might be a little more onerous...

You're right, the openRPC document is JSON, so yes we can certainly return a subset of the document.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should just be a rejected request then.

OK I'll push another commit aligned with this different behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should just be a rejected request then.

OK I'll push another commit aligned with this different behavior.

Cool, we should use this discussion to further explain what's going on and provide context for the code example.

CAIPs/caip-217.md Outdated Show resolved Hide resolved
CAIPs/caip-217.md Outdated Show resolved Hide resolved
@bumblefudge bumblefudge merged commit e4fe918 into master May 19, 2023
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.

5 participants