-
Notifications
You must be signed in to change notification settings - Fork 321
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
CIP-0095 | signData correction #897
base: master
Are you sure you want to change the base?
Conversation
- Clean up duplicate definitions over CIP-30. No need to re-define what is already defined in CIP-30, only whats changed/how it extends existing. - `address` header should be the drep hash and not a type 6 constructed address from drep id.
Considering an Update in the future: Should we change CIP-0095 in a way that the CIP-129 Hash representation is used for the address field in the COSE_Sign1 structure? Its ok to use the Hash (which represents the DRep-ID/CC-Hot/CC-Cold) in the address field. However, you can't tell what type it really is. So switching to CIP-129 here would be handy, because it would allow a check on the type of hash/id returned. However, this is not implemented yet in HW-Wallets or cardano-hw-cli or so... |
Though not a bad idea, for now I think we should stick to CIP-5 as CIP-129 haven't been accepted yet (hopefully soon). Or park this one as a dependency on CIP-129 and modify it to reference to that one for address payload. There is also the HW issue you mentioned that will cause issues. CIP-95 also only extend signData to take a DRep ID, not CC. |
@Scitz0 i mean its all backward compatible, if the address field is only a 56char return, its a simple hash. if its 58char, its a payment or stake address (or goverment ids CIP-129) |
True, this is how we deal with it in Koios and Eternl. However, when verifying a signature through some of the signData libraries out there, it could cause a problem if those libraries dont deal with all formats. And if this specification only mention CIP-129 they might not add the backwards compatible support. I dont see a good way around that though and those lib creators will just have to adapt. To be fair, just checked one but CF version seem to expect an address and not just the raw hash as implemented by Ledger and now other, so wont work with just hash as is anyway without a change. |
As they are using libs from strica (ashish), who are also the CIP-129 proposer, that might be an easy fix. But yea... lets check this out in the upcoming weeks. CIP-129 uses 2 & 3 for the "networkid", so its detectable by a lib. |
@ashisherc would it be possible from your side to update the libs to also be cip-129 compatible without interfering with existing usage? |
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 seems sensible & well enough expressed to me (and removes the redundancy with CIP30), though I would leave it to @Ryun1 and/or @Crypto2099 to verify before merge (can briefly discuss this at CIP meeting in 1 hour).
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 update is Confirmed
by the last CIP meeting & awaiting further editor reviews & community discussion (thanks @gitmachtl @Scitz0).
address
header should be the drep hash and not a type 6 constructed address.Documentation isn't my best area so if you have suggestions for expressing it differently, feel free to provide suggestions. 😄
(rendered: changes begin here)