Replace publicKeyBase58 with publicKeyMultibase

[bahner]

Hepp! I believe this PR can be of use. The core says to start using multibase for encoding of keys, so I've attempted to add that. Don't be shy with civil criticisism.

[reinkrul]

See verification method properties of DID core data model: https://www.w3.org/TR/did-core/#verification-method-properties

Regarding backwards compatibility, it looks OK to me to replace the current property;

• Nuts node the PublicKeyBase58 property in 1 place, which is easily fixed
• We're not yet on 1.x, so theoretically API changes are fine
• It's only populated for Edward-curves anyways

[reinkrul]

We use assert/require.NoError instead of nil check and t.Fatal (failure message is optional, but feel free to add the,).

[woutslakhorst]

Thanks for the suggestion and work. I would suggest to keep PublicKeyBase58 in the VerificationMethod struct so we can always parse it (backwards compatible) and use the PublicKeyMultibase for adding new keys (via NewVerificationMethod).

This would require some added logic since either PublicKeyBase58 or PublicKeyMultibase must be filled, but not both.

[reinkrul]

I would suggest to keep PublicKeyBase58 in the VerificationMethod struct so we can always parse it (backwards compatible) and use the PublicKeyMultibase for adding new keys (via NewVerificationMethod).

The spec now only specifies the multibase variant, so I'm fine with removing the base58 variant.

[woutslakhorst]

I would suggest to keep PublicKeyBase58 in the VerificationMethod struct so we can always parse it (backwards compatible) and use the PublicKeyMultibase for adding new keys (via NewVerificationMethod).

The spec now only specifies the multibase variant, so I'm fine with removing the base58 variant.

Then the lib MUST go to a new major version and you have to support 2 versions

[bahner]

Hepp! I have had a few busy days at work. Fixed the bad test. Should be in accordance with @reinkrul wishes now. I understand @woutslakhorst objection. I thought about and assumed a major bump. But as @reinkrul stated elsewhere - we're not at 1.0 yet.

I'm good with either, but feel that supporting a deprecated version before 1.0 is being too nice. from my POV changes have finished as requested.

[woutslakhorst]

We're running the current version in production and there's no way we can move towards the new format.

[woutslakhorst]

We're running the current version in production and there's no way we can move towards the new format.

I was mistaken, We only use PublicKeyJwk in production. However, even if we're not directly affected, others might. The best way is to add PublicKeyMultibase and deprecate PublicKeyBase58 with a message that it'll be removed in v1.x.y

[bahner]

Im sorry. I don't know how to solve that issue. Thanks for taking the time to review my suggestions.

[reinkrul]

I'll give it a go.

[bahner]

Il miglior fabbro

[reinkrul]

Im sorry. I don't know how to solve that issue. Thanks for taking the time to review my suggestions.

You deleted your branch, so I can't clone it. Do you still have it locally and can you push it again?

[bahner]

Oops. No sry. But the PR still shows the diffs and such, so I suspect there is a copy in the pull request. The changes were not very big, so It's just a little copy and paste, I suspect. - bahner tir. 26. sep. 2023 kl. 09:07 skrev reinkrul ***@***.***>:

…

Im sorry. I don't know how to solve that issue. Thanks for taking the time to review my suggestions. You deleted your branch, so I can't clone it. Do you still have it locally and can you push it again? — Reply to this email directly, view it on GitHub <#82 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNLSL24F3LQKXCBDU3GTS3X4J5LTANCNFSM6AAAAAA4SHAGIE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

-- Mvh, Lars Bahner

[reinkrul]

Opened #86