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

Adjusting CAIP-171 as per last meeting #173

Merged

Conversation

bumblefudge
Copy link
Collaborator

@bumblefudge bumblefudge commented Nov 16, 2022

  • generalize "token" to "identifier" to be more platform-neutral
  • made assumptions more explicit
  • add stability assumption (identifier doesn't change as session state changes)

NB @ritave (can't tag you for review directly)

partly addresses #141 , although #170 is probably the place to further specify session management for shared assumpions. ideally the choice to use #170 session objects would be separate from the choice to use #171 session identifiers, and wallets or users would know what they can assume based on one or both of the standards being conformed to?

Copy link
Contributor

@ritave ritave left a comment

Choose a reason for hiding this comment

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

Generally looks good with minor asks

1. It MUST uniquely identify an open and stateful session.
1. It MUST identify a closeable session, and it MUST become invalid
after a session is closed.
1. It MUST remain the same as the identified session's state changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the identifier remains constant when the state changes, i.e. not content-addressable/hash-based. better phrasing appreciated!

## Copyright

Copyright and related rights waived via
[CC0](https://creativecommons.org/publicdomain/zero/1.0/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'd suggest having the CC0 in the LICENSE file of this repo as well.

That's what we do in Snaps Improvement Proposals


n/a
- 2022-11-26: add mandatory indexing by session identifier (i.e. CAIP-171 requirement)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a Draft, imho the changelog shouldn't be in it until it's Final, where we could add Errata.

Bumblefudge and others added 3 commits November 16, 2022 17:31
caip: 171
title: Session Identifiers
author: Olaf Tomalka (@ritave)
discussions-to: <URL>
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
discussions-to: <URL>
discussions-to: https://github.com/ChainAgnostic/CAIPs/discussions/176

@bumblefudge bumblefudge merged commit 3924966 into ChainAgnostic:master Nov 22, 2022
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.

2 participants