CIP-0095 | Misspelling of DeprecatedCertificate

[nilscodes]

Minor wording improvements to the standard committed alongside.

[rphair]

It is definitely correct to use the technical term deprecated rather than depreciated here, but now that this spec has been published for a while we also need to confirm with @Ryun1 that correcting the spelling won't cause compatibility issues. cc @Crypto2099

Classifying this as an Update since we cannot be sure yet that this would not be a behavioural change.

[rphair]

@nilscodes I'm backing out my previous ✅ for these changes until we can be sure you are done force-pushing to this branch. Please post here when you're done making changes and then we'll get all the editors to make an assessment of it.

force push makes approval no longer relevant

[nilscodes]

Hey Robert - yeah, I had already reached out to @Ryun1 to talk about this. To be honest, wanted to make a draft into my own repo before making the official MR, but GitHub snuck up on me. There will be no further force pushing, and I'll update y'all here on the conversations with Ryan, he said he might reply directly, too.

[Ryun1]

Great catches @nilscodes
thanks for taking the time and creating this PR

I do worry that changing the error codes could break existing compatibility, although more likely to break dApp handelling of the errorcode than wallets
I will reach out to known implementors and make sure that this is okay (tagging a few @Scitz0, @refi93, @mchappell, @mirceahasegan)

[nilscodes]

Great catches @nilscodes thanks for taking the time and creating this PR

I do worry that changing the error codes could break existing compatibility, although more likely to break dApp handelling of the errorcode than wallets I will reach out to known implementors and make sure that this is okay (tagging a few @Scitz0, @refi93, @mchappell, @mirceahasegan)

Sounds good. I've already reached out to Eternl, since their non english speaking nature may not have alerted them to the misspell. Also, it may be that the implementers used the correct spelling and did not even realize the misspell in the CIP, leading to inconsistent implementations.

I just pushed some more changes, based on the clarity of the DRepID / DRepPublicKey ordering in CIP-0105 to clarify one depends on the other, and proposed a clarification in the first paragraph as indicated by my comment before. Let me know if that works.

[Scitz0]

We will/have made the change in error response to DeprecatedCertificate in Eternl. We just used error codes as defined in CIP initially even though I fully agree with the change.

[Scitz0]

I just pushed some more changes, based on the clarity of the DRepID / DRepPublicKey ordering in CIP-0105 to clarify one depends on the other, and proposed a clarification in the first paragraph as indicated by my comment before. Let me know if that works.

How does this align with CIP-129 regarding DRep ID definition?
I guess that's its own CIP and should not impact this one.

[nilscodes]

CIP-129

Yeah, I have not changed any of the definitions, just reordered them to match CIP-0105 (because the DRepID is derived from the DrepPubKey). And I don't believe CIP-0129 contains any references to the error codes. Thanks for getting back on this so quickly 🙏

~ Nils

[Ryun1]

How does this align with CIP-129 regarding DRep ID definition?
I guess that's its own CIP and should not impact this one.

Since CIP95 returns just pub keys, without any encoding/ formatting or hashing
so if dApps choose to apply CIP129, it shouldnt impact CIP95

[Crypto2099]

This looks alright to me as written. Deprecated is definitely the correct word to use in this instance and it doesn't seem like this would break many implementations and it looks like most of the requisite wallet and tooling creators have already been tagged in and could react to these changes rather easily (it would seem).

[mirceahasegan]

Great catches @nilscodes thanks for taking the time and creating this PR

I do worry that changing the error codes could break existing compatibility, although more likely to break dApp handelling of the errorcode than wallets I will reach out to known implementors and make sure that this is okay (tagging a few @Scitz0, @refi93, @mchappell, @mirceahasegan)

No issue from Lace side. Thank you for tagging me!

[mirceahasegan]

Nice work @nilscodes 👏

[Scitz0]

As this one hasn't been merged yet, I propose another change to CIP-95 regarding signData.

Its currently defined as:

To construct an address for DRep Key, the client application should construct a type 6 address. Using an appropriate Network Tag and a hash of a public DRep Key.

This is NOT how for example Ledger has implemented signData. No address %b0110 | NETWORK-TAG prefix is added to the protected header address field, just the raw key hash bytes. I think this makes sense. Why try to treat a key, be it pool, drep, cc_hot or cc_cold as an address when its not?

[rphair]

that sounds like a good point (#875 (comment)) @Scitz0 - can you submit a PR for it, or open an issue if you think the approach to such a change is debatable or needs clarification / confirmation first?

[Scitz0]

that sounds like a good point (#875 (comment)) @Scitz0 - can you submit a PR for it, or open an issue if you think the approach to such a change is debatable or needs clarification / confirmation first?

I would, if it weren't for the fact that I leave for a 1½ week vacation in the morning. So if someone else has the time, feel free to do so... else it will have to wait a bit.

[nilscodes]

that sounds like a good point (#875 (comment)) @Scitz0 - can you submit a PR for it, or open an issue if you think the approach to such a change is debatable or needs clarification / confirmation first?

I would, if it weren't for the fact that I leave for a 1½ week vacation in the morning. So if someone else has the time, feel free to do so... else it will have to wait a bit.

I would do it, if I felt confident I knew exactly what to change, but I'm not deep enough in the material unfortunately. Maybe @Ryun1 can help out?

[rphair]

@nilscodes it seems proper based on the last #875 (comment) to tag this Waiting for Author (and we do have to keep it tagged something) but @Ryun1 we are also waiting for you to advise 100% one way or the other about whether to merge this... avoiding compounded difficulties, which I think are not likely to get better as long as the misspelling persists... or disqualifying the correction in the name of backward compatibility.

[nilscodes]

@nilscodes it seems proper based on the last #875 (comment) to tag this Waiting for Author (and we do have to keep it tagged something) but @Ryun1 we are also waiting for you to advise 100% one way or the other about whether to merge this... avoiding compounded difficulties, which I think are not likely to get better as long as the misspelling persists... or disqualifying the correction in the name of backward compatibility.

Since the comment from @Scitz0 was not primarily related to my changes, not sure this MR should wait for it - here we really should finalize if we change the error codes or not. Not a lot of tooling is using any of this yet, most affected would be wallets and potentially MeshSDK.

[Scitz0]

Since the comment from @Scitz0 was not primarily related to my changes, not sure this MR should wait for it - here we really should finalize if we change the error codes or not. Not a lot of tooling is using any of this yet, most affected would be wallets and potentially MeshSDK.

Agree, I created a separate PR for that change now here #897.

[rphair]

thanks @nilscodes for that confirmation and @Scitz0 for separating & progressing that other issue. We should indeed decide on this ASAP and commit to something soon so I've marked it Last Check for the CIP meeting in about 1 hour (https://hackmd.io/@cip-editors/96).

cc @Ryun1 (waiting for his approval re: side effects of making the correction)

[Ryun1]

Thank you @nilscodes !