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

ERC: µTopic Delegation #2502

Closed
wants to merge 6 commits into from
Closed

ERC: µTopic Delegation #2502

wants to merge 6 commits into from

Conversation

3esmit
Copy link
Contributor

@3esmit 3esmit commented Feb 2, 2020

@axic axic added the type: ERC label Aug 28, 2020
@github-actions
Copy link

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP 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 stale label Oct 28, 2020
@3esmit
Copy link
Contributor Author

3esmit commented Oct 29, 2020

Yes, I would like to move forward this EIP.

@github-actions github-actions bot removed the stale label Oct 29, 2020
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

CI is blocked by the improper link format for EIP-2470. The rest of the feedback isn't critical for merging as a draft, though I strongly encourage looking it over and addressing it.

Comment on lines +16 to +18
A common trust network for ethereum.

A general purpose hierarchy of delegations which can divided into micro topics.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A common trust network for ethereum.
A general purpose hierarchy of delegations which can divided into micro topics.
A general purpose hierarchy of delegations which can divided into micro topics.

Simple Summary should be akin to an email subject line, forum thread title, GitHub Pull Request title line, etc.

Comment on lines +22 to +26
When "everything" becomes DAOable, it could become a problem to keep track of all the micro DAOs. Just like ERC20 have an allowance for other address spent on their behalf,
Cold wallets usually don't want to be unlocked very often to be able to vote.
Users might want to trust all their DAOs to other address for vote for them.
A group of people want to trust each other to vote in topics.
DAOs might want to inherit the _root_ delegate of users for their own _topic_, where users could define a specific delegate..
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When "everything" becomes DAOable, it could become a problem to keep track of all the micro DAOs. Just like ERC20 have an allowance for other address spent on their behalf,
Cold wallets usually don't want to be unlocked very often to be able to vote.
Users might want to trust all their DAOs to other address for vote for them.
A group of people want to trust each other to vote in topics.
DAOs might want to inherit the _root_ delegate of users for their own _topic_, where users could define a specific delegate..

This all sounds like great content for the Motivation section, but not really appropriate for the Abstract section. The abstract should be a human readable form of the specification. Someone should be able to read the abstract and get the gist of the specification itself. Someone should be able to read the motivation section and get an idea of why someone may want to implement this specification.

Comment on lines +40 to +44
```plantuml
@startuml
:A: -> :A:
@enduml
```
Copy link
Contributor

Choose a reason for hiding this comment

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

We should find out if UML is supported in all of the places this is rendered. GitHub, EIPs site, anywhere else? If not, then this syntax likely is going to be more confusing than helpful.


### Influence flow

The delegate which gets the influence is the first on the chain that vote, in the example above, if only `E` voted, it would accumulate the influence of all others and itself, `A,B,C,D,E,F,G,I`, while if `D` and `G` voted, then `D` would be able to claim influence of `A,B,I,C,D`, while `G` would claim from `E,F,H,G`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The delegate which gets the influence is the first on the chain that vote, in the example above, if only `E` voted, it would accumulate the influence of all others and itself, `A,B,C,D,E,F,G,I`, while if `D` and `G` voted, then `D` would be able to claim influence of `A,B,I,C,D`, while `G` would claim from `E,F,H,G`.
The delegate which gets the influence is the first on the chain that vote, in the example above, if only `E` voted, it would accumulate the influence of all others and itself, `A,B,C,D,E,F,G,H,I`, while if `D` and `G` voted, then `D` would be able to claim influence of `A,B,I,C,D`, while `G` would claim from `E,F,H,G`.

Comment on lines +197 to +199
/**
* @author Ricardo Guilherme Schmidt (Status Research & Development GmbH)
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Since everything in the EIPs repository is CC0, attribution is not required and just adds noise to the interface. Recommend removing.

Also, we are trying to minimize people using EIPs as a form of advertising their product/company, and one strategy to assist with that is strongly discouraging/disallowing people linking externally or referencing external products/businesses.

Comment on lines +250 to +252
/**
* @author Ricardo Guilherme Schmidt (Status Research & Development GmbH)
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion as above.

Comment on lines +245 to +416
*/
function delegatedTo(address _who)
external
view
returns (address)
{
return findDelegatedToAt(_who, block.number);
}

/**
* @notice Reads `_who` configured delegation at block number `_block` in this level,
* or from parent level if `_who` never defined/defined to parent address.
* @param _who What address to lookup.
* @param _block Block number of what height in history.
* @return The address `_who` chosen delegate to.
*/
function delegatedToAt(
address _who,
uint _block
)
external
view
returns (address directDelegate)
{
return findDelegatedToAt(_who, _block);
}

/**
* @dev Changes the delegation of `_from` to `_to`.
* If `_to` is set to 0x00, fall to parent proxy.
* If `_to == _from` removes delegation.
* @param _from Address delegating.
* @param _to Address delegated.
*/
function updateDelegate(address _from, address _to) internal {
emit Delegate(_from, _to);
DelegateSet memory _newFrom; //allocate memory
DelegateSet[] storage fromHistory = delegations[_from];

//Add the new delegation
_newFrom.fromBlock = uint96(block.number);
_newFrom.to = _to; //delegate address

fromHistory.push(_newFrom); //register `from` delegation update;
}

/**
* @dev `_getDelegationAt` retrieves the delegation at a given block number.
* @param checkpoints The memory being queried.
* @param _block The block number to retrieve the value at.
* @return The delegation being queried.
*/
function getMemoryAt(DelegateSet[] storage checkpoints, uint _block) internal view returns (DelegateSet memory d) {
// Case last checkpoint is the one;
if (_block >= checkpoints[checkpoints.length-1].fromBlock) {
d = checkpoints[checkpoints.length-1];
} else {
// Lookup in array;
uint min = 0;
uint max = checkpoints.length-1;
while (max > min) {
uint mid = (max + min + 1) / 2;
if (checkpoints[mid].fromBlock <= _block) {
min = mid;
} else {
max = mid-1;
}
}
d = checkpoints[min];
}
}

/**
* @notice Reads `_who` configured delegation at block number `_block` in this level,
* or from parent level if `_who` never defined/defined to parent address.
* @param _who What address to lookup.
* @param _block Block number of what height in history.
* @return The address `_who` chosen delegate to.
*/
function findDelegatedToAt(
address _who,
uint _block
)
internal
view
returns (address directDelegate)
{
DelegateSet[] storage checkpoints = delegations[_who];

//In case there is no registry
if (checkpoints.length == 0) {
return (
address(parentDelegation) == address(0) ?
address(0) : parentDelegation.delegatedToAt(_who, _block)
);
}
return getMemoryAt(checkpoints, _block).to;
}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving this to ## Implementation section, as it doesn't define the public surface area, which is really what is critical for the specification.

Comment on lines +421 to +425
Democracies are seen as a multisig of infinite participants, which signatures weight are on their voting token and the threshold as usually 50%+1 or more.

Being possible to approve something in qualified quorums becomes more difficult the wider the community of holders is, and approving very specific tasks can be complicated to evaluate for most of holders.

Therefore the design was around solving the problem of defining delegates in a common but optionally specialized way building a trust network which can easily and carefully approve proposals.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is all content better suited for the Motivation section. The Rationale section should describe why you made specific choices, like why did you decide to deploy at a deterministic address, or why did you decide to use a salt, or why a particular obviously desirable feature was not included or why a non-obvious feature was included.

Comment on lines +456 to +459
## References

https://docs.google.com/presentation/d/1FJkaJlp_Fs0D5KHsn8oEgDOhg9QKiBwxHOvUL0dvC5I/edit#slide=id.g50bded6ab0_0_10

Copy link
Contributor

Choose a reason for hiding this comment

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

EIPs don't have a References section, which is intentional. EIPs should be able to stand on their own without external reference material. If there is some critical content for the EIP that must be included (forever) then it should be added as an asset in ../assets/eip-2502/ and linked as part of one of the other sections.

EIPS/eip-2502.md Outdated Show resolved Hide resolved
@github-actions
Copy link

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP 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 stale label Dec 30, 2020
Co-authored-by: Micah Zoltu <micah@zoltu.net>
EIPS/eip-2502.md Outdated Show resolved Hide resolved
@3esmit
Copy link
Contributor Author

3esmit commented Jan 4, 2021

Please, keep it open.

Co-authored-by: Micah Zoltu <micah@zoltu.net>
@MicahZoltu
Copy link
Contributor

MicahZoltu commented Jan 4, 2021

Please, keep it open.

@3esmit You are saying you don't want this merged as a draft now that CI is passing? We can do that for a couple days, but the normal process is to get things merged as draft as soon as they are properly laid out and then the author (you) can iterate on the draft with auto-merges.

@3esmit
Copy link
Contributor Author

3esmit commented Jan 4, 2021

Yes, I still need to address all the things here before going to draft.

@github-actions github-actions bot removed the stale label Jan 4, 2021
@github-actions
Copy link

github-actions bot commented Mar 5, 2021

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP 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 stale label Mar 5, 2021
@github-actions
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants