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-0036 | Change reward address field for Fund 10 #373

Conversation

Ryun1
Copy link
Collaborator

@Ryun1 Ryun1 commented Nov 11, 2022

Proposed Change

Replace reward_address with payment_address. Where payment_address should be a Shelley payment address. This means that the payment of voting rewards in Catalyst can no longer be done via MIR transfer but rather require regular transactions.

Motivation

Past Catalyst's Fund 9, MIR transfers will be unavailable to be used by Project Catalyst. The operators of Project Catalyst, IOG, do not control the keys necessary to perform MIR transfers. This aligns Catalyst with all other projects implementing this standard.

Historically the use of MIR transfers by Project Catalyst was allowed to save on the costs associated with the payment of thousands of voting rewards. But this application was beyond the intended design of the mechanism since Project Catalyst is not a core network function.

The removal of this constraint makes the application of this standard more practical for projects beyond Catalyst to adopt.

Note: In an effort to ease the transition and not remove voting rights unexpectedly from already registered voters, Catalyst's Fund 10 registrations/delegations with the incorrect type of reward_address will still be valid, but will not be eligible to receive voting rewards.

cc: @kevinhammond @stevenj @ehanoc

@gitmachtl
Copy link
Contributor

Registrations/delegations with the incorrect type of rewards_address will still be valid, but will not receive voting rewards.

Why should an incorrect type still be valid?

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Nov 14, 2022

Why should an incorrect type still be valid?

Good question. The idea is that its a lesser sin to not get a reward for voting than be denied your voting rights, due to a reward address change outside of your control, that you may have not found out about until after its too late.

Registrations should entitle you to vote. IF the rewards address is something we can pay to, AND you earn a reward, then a reward gets paid to it.

(forwarded answer from @stevenj)

@Ryun1 Ryun1 marked this pull request as ready for review November 16, 2022 14:43
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
@gitmachtl
Copy link
Contributor

lgtm

@GitCardano
Copy link

How are you going to explain this change to those of us who struggle with change? I just figured out how to vote, Fund9 was my first. What is "regular payment address"? I hold my Cardano on Daedalus.

@gitmachtl
Copy link
Contributor

How are you going to explain this change to those of us who struggle with change? I just figured out how to vote, Fund9 was my first. What is "regular payment address"? I hold my Cardano on Daedalus.

don't worry. daedalus will get an update in time.

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Nov 27, 2022

How are you going to explain this change to those of us who struggle with change? I just figured out how to vote, Fund9 was my first. What is "regular payment address"? I hold my Cardano on Daedalus.

@GitCardano - This change is to be implemented by wallets and supporting tools. End users/voters should not have to do anything but keep their wallet/tool of choice updated.

@rphair rphair added the Update Adds content or significantly reworks an existing proposal label Nov 27, 2022
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.

My reading of the dialogue so far is that all reservations have addressed so I would readily approve this, pending:

  • @KtorZ - I assume will want to give it a good technical review (I'm not qualified to determine if there could be any hang-ups in doing it this way).
  • @Ryun1 - Is there a way we can verify that IO > Catalyst team actually intends to do things this way? (see below)

(edit, added) Although this requires changes to wallets in production I would forego the usual "Path to Active" because with IO's official endorsement it seems that wallets starting with Daedalus would be forced to implement the change quickly & as a matter of course.

@@ -230,6 +230,10 @@ Fund 8:
- added the `voting_purpose` field to limit the scope of the delegations.
- rename the `staking_pub_key` field to `stake_credential` and `registration_signature` to `registration_witness` to allow for future credentials additions.

Fund 10:
- stipulated that `reward_address` must be a Shelley payment address, otherwise voting reward payments will not be recieved.
- **Note:** up to Fund 9 `reward_address` was a Shelley rewards address; from Fund 10 onwards, it will be a normal payment address. This will allow rewards to be paid directly from the designated Catalyst pot rather than requiring MIR transfers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a link to some Catalyst documentation that verifies this change? If no link exists, could we add some background here about the Catalyst team's decision to do this?

Copy link
Member

Choose a reason for hiding this comment

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

This will allow rewards to be paid directly from the designated Catalyst pot

What is, the "designated Catalyst pot" ? I thought this pot was precisely called the "treasury" 😶 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@KtorZ from my reading of the CIP currently, Fund 10 will allow (or be based on) paying from accounts that are not the treasury. But @Ryun1 there is still a need to document this change. The Catalyst changes are usually documented on a Git Doc somewhere & I still believe we should have a link to this resolution in the CIP text.

Copy link
Collaborator Author

@Ryun1 Ryun1 Dec 6, 2022

Choose a reason for hiding this comment

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

@KtorZ @rphair
Im working on fleshing out the motivation for this.

Copy link
Contributor

@stevenj stevenj Jan 4, 2023

Choose a reason for hiding this comment

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

@rphair I think there is a misunderstanding here. Proj Catalyst did not make a decision to make this change. Proj Catalyst was forced to by circumstance and directive. IOG does not control the keys sufficient to perform MIR transfers, so its incapable of performing them. The only documentation proj catalyst has on registration and delegation is CIP-15 and CIP-36. The motivation therefore is "Project Catalyst would like to continue to have the option of paying voter rewards. Now that MIR transfers are unavailable for that purpose, the only option is to change the reward address to a normal payment address. This also has the added benefit that these formats can be utilized by other applications, which is what the CIP-36 specifically envisioned when it added a voting purpose to the record. In recognition that this change may cause a large number of previously registered voters to now have an 'invalid' registration, the prudent course would seem to be not denying a voter the right to vote, simply because they would be unable to receive a voter reward through no fault of their own.". Or words to that effect.

Copy link
Collaborator

@rphair rphair Jan 4, 2023

Choose a reason for hiding this comment

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

thanks @stevenj, that was my understanding (though not including the detail about the inadequate keys)... once we get confirmation @Ryun1 is done working on the Motivation as it appears in the CIP then I'll double check that all your points are clarified there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rphair

See, updated motivation section :)

Copy link
Collaborator

@rphair rphair Jan 9, 2023

Choose a reason for hiding this comment

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

thanks for updates @Ryun1 but so far I'm only seeing the Motivation section of the CIP draft has had "address" changed to "payment address" & no other changes. @stevenj's comments are now reflected in the original post of this PR but I don't see them in the CIP document yet... have I missed something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rphair

Apologies I was unclear, poor choice of words - I was trying to say that @stevenj's comments have been reflected in this PR's motivation.

CIP-0036/README.md Outdated Show resolved Hide resolved
CIP-0036/README.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
Co-authored-by: Ján Mazák <[email protected]>
@@ -110,7 +110,7 @@ considered valid if the following conditions hold:
The advised way to construct a nonce is to use the current slot number.
This is a simple way to keep the nonce increasing without having to access
the previous transaction data.
- The reward address is a Shelley address discriminated for the same network
- The reward address is a Shelley payment address discriminated for the same network
Copy link
Member

Choose a reason for hiding this comment

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

That sentence reads funny now 🙃 ... "reward_address is a payment address". Naming things is hard I guess.

-> Might be good to consider renaming the field altogether.

Copy link
Contributor

@gitmachtl gitmachtl Dec 1, 2022

Choose a reason for hiding this comment

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

maybe we should change it here and in the cddl from rewards_address to voting_rewards_payout_address. or remove the wording "rewards" in total and just call it voting_payout_address or even just payout_address.

Copy link
Collaborator Author

@Ryun1 Ryun1 Dec 28, 2022

Choose a reason for hiding this comment

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

I'm on the fence about renaming it, I would like further input.

Historically, before Catalyst's Fund 4, this field was a payment address and was named reward_address, see #89 - it does not appear to have been an issue back then. With that said, I do understand additional the potential for confusion as this is not the first time this field has changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@KtorZ @gitmachtl

See most recent commit, I have renamed the field to just payment_address. In addition to making things clearer, I believe removing the notion of rewards from the field name makes the specification less Catalyst specific; as the notion of voting rewards are Catalyst specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ryun1 hey, yes thx i have seen it and i think its ok. but tbh, i have not really seen a problem calling it rewards_address too. But its clear now IMO.

Co-authored-by: Ján Mazák <[email protected]>
@Ryun1 Ryun1 changed the title CIP-0036 | Change rewards address field for Fund 10 CIP-0036 | Change reward address field for Fund 10 Jan 5, 2023
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
CIP-0036/test-vector.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Jan 20, 2023

@Ryun1 can you rebase & merge from upstream master to fix the current merge conflict (from #426 being merged in the meantime)? cc @KtorZ

(p.s. thanks... looks like done now & our last actions were at precisely the same time.)

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.

Looks to me like last round of editing covers everything & I will leave it to @KtorZ & maybe also @SebastienGllmt to double-check.

@KtorZ KtorZ merged commit a98b5fa into cardano-foundation:master Jan 20, 2023
Ryun1 added a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
…n#373)

* Stressed payment address for reward payments

* Adjusted wording and started with test vectors

* Reset changes to test vectors

* Added test vectors

* Typos + added public gov key to test vectors

* Missed comma

* Update CIP-0036/README.md

Co-authored-by: Ján Mazák <[email protected]>

* Update CIP-0036/README.md

Co-authored-by: Ján Mazák <[email protected]>

* Removed cardano-cli specific notation from test vectors, just leaving CBOR hex

* Renamed rewards_address field to payment_address

* Addressed @KtorZ comments

Co-authored-by: Matthias Benkort <[email protected]>
Co-authored-by: Ján Mazák <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants