Skip to content
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-0129? | Governance Identifiers #857

Merged
merged 21 commits into from
Nov 15, 2024

Conversation

ashisherc
Copy link
Contributor

@ashisherc ashisherc commented Jul 16, 2024

Introduces byte representation along with a bech32 prefixes for different Conway era credentials and identifiers.

This PR includes relevant updates to CIP - 0005, and 0105 (including updated test vector)


(rendered proposal)

@Ryun1 Ryun1 changed the title add CIP for gov identifiers CIP-???? | Governance Identifiers Jul 16, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashisherc thanks; looks like a great start & looking forward to introducing this at the next CIP meeting & hope you can be there for an initial discussion: https://hackmd.io/@cip-editors/93

CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
@Ryun1 Ryun1 changed the title CIP-???? | Governance Identifiers CIP-129? | Governance Identifiers Aug 1, 2024
CIP-XXXX/README.md Outdated Show resolved Hide resolved
@rphair rphair changed the title CIP-129? | Governance Identifiers CIP-0129? | Governance Identifiers Aug 1, 2024
@rdlrt
Copy link

rdlrt commented Aug 2, 2024

Once this is merged, We would begin work for supporting it for CNTools and Koios API

@gitmachtl
Copy link
Contributor

gitmachtl commented Aug 2, 2024

SPO Scripts can support it too. But CLI and DB-Sync must also be onboard. For HW-Wallets, such a change will most likely not be implemented until the end of the year, the release cycle from the vendors (not the devs) is horribly slow nowadays. So, the sooner this gets merged and into the pipelines, the better. 👍

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashisherc it sounds like you can put the items provided by @rdlrt @gitmachtl on the Implementation plan, which will fill out that section to a bare minimum for continuing review. It needed to be more specific anyway and I think it's even better to have specific ranges of work to focus on.

I would recommend tickboxes for the general categories (operator tooling, developer APIs) and then the projects above as specific examples in tickboxes nested under them.

Also based on @gitmachtl's assessment about hardware wallet support being slow, I think it would help to highlight that in the implementation plan: as a task to bridge with the responsible developers. Therefore I think that section would also be better suited as tickboxes because of the list of things that are already mentioned there (anything done already can be pre-ticked).

If we have an updated Path to Active before Tuesday we can get this again on the CIP meeting agenda & keep this moving along toward merge. Thanks @rdlrt @gitmachtl for the community review & offers of support.

@ashisherc
Copy link
Contributor Author

@rphair updated the PR with the implementation plan, please provide context/recommendation for acceptance criteria.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please provide context/recommendation for acceptance criteria

According to #857 (review) (and pending further recommendations by @Ryun1 @Crypto2099)...

CIP-0129/README.md Outdated Show resolved Hide resolved
@dmitrystas
Copy link

good stuff, AdaStat will definitely support this CIP

@Ryun1
Copy link
Collaborator

Ryun1 commented Aug 6, 2024

Although I do think this proposal adds values to Cardano tooling for users.

I believe at this stage of maturity for governance tooling having competing standards might not be ideal, potentially damaging user experience and compatibility.
I am worried that we reach the Chang hardfork (estimated end of August) and half of tooling represents DRep IDs one way and the other half of tooling represents it another way.
This would likely cause users (and implementors) lots of issues within their first experience of Cardano's community governance, potentially impacting the legitimacy of the system.

Id suggest we move to make this standard an "extension" to CIP105, so that it optionally adds to the existing standard, in a backwards compatible way.
This will at least give implementers a choice to upgrade to this or not.
Ideally we also make the identifiers that the users see, clearly differ from existing ones to avoid confusion.

@ashisherc
Copy link
Contributor Author

ashisherc commented Aug 8, 2024

@Ryun1

Id suggest we move to make this standard an "extension" to CIP105, so that it optionally adds to the existing standard, in a backwards compatible way

After extensive discussions on CIP105, it was determined that this specification should be its own CIP, as it is not related to CIP105 (which specifies the derivation for new Conway era keys). Confusing this CIP as a backward-compatible extension of CIP105 is a mistake. This CIP establishes a new specification for various Conway era credentials generated using CIP105.

I believe at this stage of maturity for governance tooling having competing standards might not be ideal, potentially damaging user experience and compatibility.

As specified in the CIP, no existing toolings need to do extensive work to support this CIP, they would only have to switch to a proper bech32 prefix. I do not believe it is too much work just to replace drep -> drep_vkh for existing tools. I wouldn't want to create so much chaos but the suggested new prefix aligns with the specification of the CIP105 itself.

@rdlrt
Copy link

rdlrt commented Aug 8, 2024

I believe at this stage of maturity for governance tooling having competing standards might not be ideal, potentially damaging user experience and compatibility.
I am worried that we reach the Chang hardfork (estimated end of August) and half of tooling represents DRep IDs one way and the other half of tooling represents it another way.

If this has to go through, sooner the better, to prevent the exact scenario mentioned. Currently the support for conway across tooling is a bit premature, with all exploration of edge cases requiring frequent fixes/changes. More mature the tooling becomes, more resistance there would be for changes later on.

@gitmachtl
Copy link
Contributor

@ashisherc is about to update it like i described above.

@Ryun1
Copy link
Collaborator

Ryun1 commented Nov 12, 2024

Hi all,
Appreciate the discussion on this.

Context

  • CIP105 provided definition for Conway credentials, including bech32 prefixes
    • Prefixes for DRep credentials and CC credentials.
  • Some tooling/wallets have implemented the CIP105 definitions
    • CIP105 definitions have seemed to broadly worked
  • A different approach to Conway credentials was proposed via CIP-129
    • These definitions provide a nicer representation for these credentials
  • CIP-129 does suggest altering the CIP-105 definitions of bech32 prefixes
    • This is an issue as it requires a change to an active CIP

Options

a) We change CIP-105 to not claim the prefixes.
b) We change CIP-129 to not claim to the prefixes.
c) We agree that CIP-105 and CIP-129 to both claim the prefixes.

My opinion

I oppose a) as it would mean tooling which is "CIP-105 compliant" would now be broken.
Changing active CIPs has proven to be a mistake time and time again.

Myself and others have suggested b) a couple of times.
This is of course at the author's discretion, and they have chosen not to take this suggestion, I think that is fine.
Although I still think it is worth considering.

With the current context I think c) is the only possible action.
With the caveat that all CIP129 compliant tools should also recognise CIP-105 prefix definitions and handle them.
c) allows for tooling to migrate between the two standards, without outright breaking CIP105 compliant tools.
This means that things could only break when users are copying CIP-129 IDs to CIP-105 tools.
And this seems to be the consensus reached by @gitmachtl + @ashisherc whilst I was writing this response.

Does anyone object to allowing both CIP105 and CIP129 claim these bech32 prefixes?
(tagging a few discussion contributors @HeinrichApfelmus, @l-br1, @vsubhuman, @rphair, @Crypto2099)

@rphair
Copy link
Collaborator

rphair commented Nov 12, 2024

@Ryun1 - from my understanding of the discussion so far (though not a tool developer) I have no objection to option (c) [CIP-105 and CIP-129 both claim the prefixes] and believe it represents the best chance of moving forward as a community over this issue.

@vsubhuman
Copy link
Contributor

@Ryun1 - from my understanding of the discussion so far (though not a tool developer) I have no objection to option (c) [CIP-105 and CIP-129 both claim the prefixes] and believe it represents the best chance of moving forward as a community over this issue.

Same. Not perfect, but seems like a way forward. At the very least it makes the CIP recognize that prior state exists and cannot just be erased from documentation.

@HeinrichApfelmus
Copy link
Contributor

@Ryun1 Thanks! This makes a lot of sense.

I don't object to c), but I do want to note has the unfortunate and large downside of bringing confusing to users — "Hey, your drep doesn't work" — "Did you use a CIP-129 or CIP-105 compliant wallet?" — "What's a CIP wallet?"?

The only solution to that is to also retire one of the standards in the long term, e.g.

d) We retire CIP-105

Doesn't have to be now.

@gitmachtl
Copy link
Contributor

As i suggested above, i am also for c).

But i would also go further and add in some information in the CIP-005 documentation for CIP105 entries, that there is now a newer version and that CIP105 might get deprecated in the future. So thats about d) @HeinrichApfelmus suggested.

@l-br1
Copy link

l-br1 commented Nov 12, 2024

Thanks @Ryun1 for the summary 🙏🏻

From my side (considering participation in Cardano's governance, DReps and delegators' needs and so needs tools should support) I would still stress that effectively creating an 'alternative' DRep ID will require DReps and delegators to be aware of both (without any real benefit and reason for them) and it will add extra unnecessary friction to an already complex experience at its early stages.

So purely considering the perspective of users of our governance system I see this as a step back.

Option C is definitely a way to unblock this easily (just looking at the CIP itself), but even from the rationale and summary in @Ryun1's comment, it doesn't seem the best option for the broader purpose.

(So I would still probably support Option B as I understand it)

@gitmachtl
Copy link
Contributor

@l-br1 in regards of user experience and that "Dreps" must be aware about an other format. In my oppinion its best to do this now, and not wait and sit it out until we have 1000+ DReps on chain.

We should not artificially delay the depoyment of a better solution. CIP129 brings bech representation for ActionIDs too. Thats very nice.

It also provides the solution to not have to deal with

drep
drep_script
cc_cold
cc_cold_script
cc_hot
cc_hot_script

credentials, but reduces that to only three from the six.

I am a bit biased about this whole topic, because i brought up this issue around 1,5 years ago. But noone reacted and it was passed and so only the direct hashes were implemented from the ledger/node/cli team. When the problem occured again, it was stated that "now its too late".

So, i hope we can move forward with this CIP and to provide a reseaonable upgrade path for the Tools that way.

@Scitz0
Copy link
Contributor

Scitz0 commented Nov 12, 2024

I vote for option c as I think it represents the best way forward from where we stand. Hopefully if accepted and merged soon, we can get broad support across tools and wallets and silently drop showing CIP-105 IDs while still allowing for backward compatibility on parsing for a time.

CIP-0129/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Crypto2099 Crypto2099 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor notes about leaving the previous prefixes in CIP-105 and flagging them as DEPRECATED instead so that we do not break any existing compatibility with this change. Everything else looks good

CIP-0105/README.md Outdated Show resolved Hide resolved
CIP-0105/README.md Show resolved Hide resolved
CIP-0105/README.md Show resolved Hide resolved
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also @ashisherc there is a merge conflict (likely for the last few weeks, since I think it's from #920) that should be resolved in your branch. ... p.s. please without a force push, so we can preserve all the pending changes & edits 🙏

CIP-0105/README.md Outdated Show resolved Hide resolved
@ashisherc
Copy link
Contributor Author

ashisherc commented Nov 14, 2024

@rphair all conflicts resolved (Pun intended)

Copy link
Collaborator

@Crypto2099 Crypto2099 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good and satisfy my feedback.

Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@Ryun1 Ryun1 merged commit 438d64d into cardano-foundation:master Nov 15, 2024
@gitmachtl
Copy link
Contributor

yea, thanks everyone involved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Tools Proposals belonging to the 'Tools' category. State: Last Check Review favourable with disputes resolved; staged for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.