-
Notifications
You must be signed in to change notification settings - Fork 134
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
[Errata] CAIP-122 table to match normative changes in PR 236 #272
Conversation
CAIPs/caip-122.md
Outdated
@@ -37,7 +37,7 @@ The data model _MUST_ contain the following fields: | |||
| Name | Type | Mandatory | Description | | |||
| ----------------- | --------------- | --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | |||
| `domain` | string | ✓ | [RFC 4501][rfc 4501] `dnsauthority` that is requesting the signing. | | |||
| `address` | string | ✓ | Blockchain address, as defined by [CAIP-10][caip-10], performing the signing; should include [CAIP-2][caip-2] chain id namespace | | |||
| `address` | string | ✓ | Blockchain address performing the signing, expressed as the `account_id` segment of a [CAIP-10][caip-10]; should NOT include [CAIP-2][caip-2] `chain_id`.| namespace | |
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.
This change is not consistent with other changes made in https://github.com/ChainAgnostic/CAIPs/pull/236/files (where string representation example depends on address
being a CAIP-10 identifier),
Rather than splitting CAIP-10 identifier into address and chain ID, I suggest renaming address
field to account_id
.
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.
This sounds good to keep it consistent with CAIP-10
paging doctor @haardikk21 cleanup on aisle editorial |
CAIPs/caip-122.md
Outdated
@@ -46,6 +46,7 @@ The data model _MUST_ contain the following fields: | |||
| `expiration-time` | string | | [RFC 3339][rfc 3339] `date-time` that indicates when the signed authentication message is no longer valid. | | |||
| `not-before` | string | | [RFC 3339][rfc 3339] `date-time` that indicates when the signed authentication message starts being valid. | | |||
| `request-id` | string | | System-specific identifier used to uniquely refer to the authentication request. | | |||
| `Chain ID` | string | ✓ | The `chain_id` segment of a [CAIP-10][caip-10], i.e., a [CAIP-2][caip-2] identifier for locating the address listed separately above.| |
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.
nit: chain-id
instead of Chain ID
here
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.
one minor nit + @silverpill's suggested change - rest looks good
@obstropolos i can't review cuz it's mine but i'll take haardik's comment above as an approval and override the rule if you give it an official approval |
CAIPs/caip-122.md
Outdated
@@ -37,7 +37,7 @@ The data model _MUST_ contain the following fields: | |||
| Name | Type | Mandatory | Description | | |||
| ----------------- | --------------- | --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | |||
| `domain` | string | ✓ | [RFC 4501][rfc 4501] `dnsauthority` that is requesting the signing. | | |||
| `address` | string | ✓ | Blockchain address, as defined by [CAIP-10][caip-10], performing the signing; should include [CAIP-2][caip-2] chain id namespace | | |||
| `account_id` | string | ✓ | Blockchain address performing the signing, expressed as the `account_id` segment of a [CAIP-10][caip-10]; should NOT include [CAIP-2][caip-2] `chain_id`.| namespace | |
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.
@bumblefudge In CAIP-10, account_id
is a whole identifier (this is what I was suggesting in my previous comment). The segment you mention here is called account_address
: https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-10.md#syntax
Conversion of CAIP-10 identifier to address and chain ID is already described at lines 76, 87 and 97:
account_address(address)
is theaccount_address
segment of a [CAIP-10][]address
of the data model
chain_id(address)
is achain_id
part of [CAIP-10][caip-10]address
of the data model.
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.
amazing how much i've confused myself with this. it now matches the other section, apologies.
making account
the whole [machine-readable] CAIP-10 doesn't work very well for clients (like all current SIWE clients) that display account to the end-user in native form (i.e. without the CAIP-2 prefix, which can be cryptic and opaque in many cases), thus the account_address only design choice to date.
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.
The text still doesn't make sense and contradicts itself.
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 agree with @silverpill I think I did it the right way in the tezos-caip-122
hopefully closes #266 . thanks again to @jdsika for pointing it out and @glitch-txs for pointing out implementation issues in the field