Check DID Identifier checksum in validation #8
Conversation
timchurchard
force-pushed
the
tc_fixes_checksum
branch
from
19159d6
to
ee54b88
Compare
timchurchard
force-pushed
the
tc_fixes_checksum
branch
from
ee54b88
to
34f0b99
Compare
timchurchard
force-pushed
the
tc_fixes_checksum
branch
from
34f0b99
to
765da1d
Compare
| @@ -83,9 +83,14 @@ def other_key_pair(other_key_pair_secrets): | |||
|
|
|||
| @pytest.fixture | |||
| def doc_did(): | |||
| return 'did:iotics:iotHHHHKpPGWyEC4FFo4d6oyzVVk6MXLmEgY' | |||
| return 'did:iotics:iotDadb3rSWedk8iqExSbwqLtijG5XQByHC7' | |||
There was a problem hiding this comment.
since this is a magic string that's repeated in many tests, would it be better to have it in a constant with a meaningful name
There was a problem hiding this comment.
Yes it would be clearer. I did not want to refactor too much of the tests. I had to override this value because it does not pass the identifier checksum validation.
| @@ -37,4 +37,5 @@ def make_identifier(public_bytes: bytes) -> str: | |||
| checksum = bytearray.fromhex(cl2.hexdigest())[:4] | |||
|
|
|||
| return IDENTIFIER_PREFIX + base58.b58encode(bytes([IDENTIFIER_METHOD, IDENTIFIER_VERSION, IDENTIFIER_PAD]) | |||
| + pk_digest + checksum).decode('ascii') | |||
| + pk_digest + checksum, | |||
| alphabet=base58.BITCOIN_ALPHABET).decode('ascii') | |||
There was a problem hiding this comment.
the use of this alphabet is fundamental - how do other impl of the spec would know about this?
would all of them need to use the same "bitcoin_alphabet" ??
There was a problem hiding this comment.
base58 was written for bitcoin so the default is "bitcoin alphabet". I only specify here in case the default in the upstream library changes and to make the code clearer to read. I'm happy to revert this override if you prefer.
| @@ -20,6 +23,17 @@ def validate_identifier(did: str): | |||
| if result is None: | |||
| raise IdentityValidationError(f'Identifier does not match pattern {did} - {IDENTIFIER_ID_PATTERN}') | |||
|
|
|||
| did_bytes = base58.b58decode(did[len(IDENTIFIER_PREFIX):], alphabet=base58.BITCOIN_ALPHABET) | |||
There was a problem hiding this comment.
following up the comment above, should this magic number be put in a constant (since it's repeated in many places or maybe have a id_sdk.did_decode/id_sdk.did_encode functions that wrap base58.b58decode/b58encode(alphabet=base58.BITCOIN_APLHABET) ?
just saying in order to have our custom logic somewhere in one place only
No description provided.