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

Add EIP-5805: Voting with delegation #5805

Merged
merged 10 commits into from
Jan 6, 2023

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Oct 20, 2022

No description provided.

@github-actions github-actions bot added c-new Creates a brand new proposal e-number Waiting on EIP Number assignment t-erc labels Oct 20, 2022
@github-actions github-actions bot removed the e-number Waiting on EIP Number assignment label Oct 20, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 20, 2022

All tests passed; auto-merging...

(pass) eip-5805.md

classification
updateEIP
  • passed!

@github-actions github-actions bot added the s-draft This EIP is a Draft label Oct 20, 2022
@Amxx Amxx marked this pull request as ready for review October 20, 2022 14:28
@Amxx Amxx requested a review from eth-bot as a code owner October 20, 2022 14:28
@Pandapip1 Pandapip1 changed the title Voting with delegation Add EIP-5805: Voting with delegation Oct 23, 2022
EIPS/eip-5805.md Outdated Show resolved Hide resolved
EIPS/eip-5805.md Outdated Show resolved Hide resolved
@Amxx Amxx dismissed a stale review via 2670277 October 24, 2022 11:56
Pandapip1
Pandapip1 previously approved these changes Oct 25, 2022
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

There's some formatting stuff that could be improved, but this is fine for a draft.

Copy link
Contributor

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

Hi @Amxx , glad to see this EIP. I think it's a super important area to address.

Left some comments of the first round.

status: Draft
type: Standards Track
category: ERC
created: 2022-07-04
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall this be update to 2022-10-19 to reflect its start time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started writing the draft of this document in july ... but waited until post-devcon to create the PR. I'm not sure what "created" is supposed to represent. I'd don't really care. If you editor thing 2022-10-19 is a better date, then I'm ok with that

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I think the nature of this is more of use the date for time it first get drafted, I don't have a preference either. Leaving it for @Pandapip1 and @SamWilsn who have more authoritative answer

EIPS/eip-5805.md Show resolved Hide resolved
EIPS/eip-5805.md Show resolved Hide resolved
inputs:
- name: account
type: address
- name: timepoint
Copy link
Contributor

Choose a reason for hiding this comment

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

The timepoint parameter should match the operating mode of the contract.

The wording is not entire clear to me. Does it require timepoint to be consistent with now()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to be consistent in the sens that:

  • if now() returns a block.number (or if now() does not exist), then timepoint should be a block number.
  • if now() returns a timestamp, then timepoint should be a timestamp.

EIPS/eip-5805.md Show resolved Hide resolved
EIPS/eip-5805.md Show resolved Hide resolved
### Solidity interface

```sol
interface IERC_XXXX {
Copy link
Contributor

Choose a reason for hiding this comment

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

[Optional] Do you want to consider adding EIP-165 identifer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, that is a good point.

I love EIP-165 but the truth is I don't see it used that often in the field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, with "now()" being optional, I'm not sure what the identifier should be?

Copy link
Contributor

@xinbenlv xinbenlv Oct 27, 2022

Choose a reason for hiding this comment

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

but the truth is I don't see it used that often in the field.

Strongly agreed with you. I am working on a proposal providing a competing identifier of EIP-165 https://eips.ethereum.org/EIPS/eip-5269 to rethink how we could make identifier useful. Happy to chat and get valueable input from you, If you @Amxx, being the main author of openzeppelin, also think that EIP-165 has been restricted of adoption, that speaks a lot.

That said, if you are not sure, maybe consider putting that into the Rationale section if you choose to or not to add EIP-165 because it's quite

Copy link
Contributor

Choose a reason for hiding this comment

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

also, with "now()" being optional, I'm not sure what the identifier should be?

Good point. It also makes restriction usage of EIP-165. I think the common approach is to make EIP-165 out of all required field, and then any optional one will have an IERC5805Extension kind of thing which has its own EIP-165 identifier.


When a "delegator" delegates its tokens voting power to a "delegatee", its balance is added to the voting power of the delegatee. If the delegator changes its delegation, the voting power is subtracted from the old delegatee's voting power and added to the new delegate's voting power. The voting power of each account is tracked through time so that it is possible to query its value in the past. With tokens being delegated to at most one delegate at a given point in time, double voting is prevented.

Whenever tokens are transferred from one account to another, the associated voting power should be deducted from the sender's delegate and added to the receiver's delegate.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you want to specify whether this is SHOULD or "MUST" for handling the transfer behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this EIP, the notion of "voting power" is purposefully open to interpretation. The most obvious case is to use the balances, but I think it is important to not be to restrictive and allow for other approaches.

In particular, I want this EIP to support quadratic voting. This means that, when an account transfers tokens to another, the new voting power of delegate might actually not change because of quadratic effects.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's totally reasonable. If it's purposefully open to interpretation, I'd suggest

  1. change the "should" to "MAY"
  2. articulate it's purposefully open to interpretation / implementation choice in the Rationale section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll stay with SHOULD .. because that what you should do unless you have a good reason not to, and not what you may do and you are not sure.

I'll add something in the Rationale section

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

@Amxx
Copy link
Contributor Author

Amxx commented Oct 31, 2022

We might need to rename the now() function, because of name conflicts with solidity's now keyword (deprecated since 0.7.0 but still raising issues).

Should we go for clock() ?

@frangio
Copy link
Contributor

frangio commented Nov 4, 2022

clock sounds good to me. Another alternative could be tick.

@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 4, 2022

clock sounds good to me. Another alternative could be tick.

Is it the intention of the function now() to represent the current point of time in either timestamp or blocknum?

I feel the tick or clock will all strongly imply timestamp and could cause trouble if contract developers choose to implement blocknum but client interpreting it as timestamp.

I'd brainstorm something like currentPointOfTime(), well that's a bit unfortunately long, please help find better name

@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 4, 2022

Also, realizing snapshot doesn't work at blocknum level. I am proposing #5875 to retrieve TX Num. Wondering if for better security, we should support TX level of voting weight snapshot

@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Dec 19, 2022
@Amxx
Copy link
Contributor Author

Amxx commented Jan 5, 2023

There is still ongoing discution about this EIP (which IMO should happen on the ethereum magician thread, and not here) ... but I believe the proposal state is sufficient to merge as a draft.

If there is anything preventing that from being merged (as a draft), please let me know what needs to be fixed.

@Amxx Amxx requested a review from Pandapip1 January 6, 2023 10:58
@Pandapip1
Copy link
Member

The only rule about the creation date is it can't be in the future.

@Pandapip1 Pandapip1 closed this Jan 6, 2023
@Pandapip1 Pandapip1 reopened this Jan 6, 2023
@eth-bot eth-bot enabled auto-merge (squash) January 6, 2023 18:40
@eth-bot eth-bot merged commit 56c5187 into ethereum:master Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants