-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 ERC-2212 markdown #2444
Add ERC-2212 markdown #2444
Conversation
72c52ee
to
5682e56
Compare
Thanks! This seems to also contains 1620, can you please remove it so that 2212 can be properly reviewed? |
5682e56
to
daf43e7
Compare
Sorted. |
daf43e7
to
db85761
Compare
|
||
Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/). | ||
|
||
@PaulRBerg and @SablierHQ. |
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.
What are these for?
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.
I saw the copyright in other standards and wrote one here too. I'll remove it if its place is not here.
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.
There is no copyright with CC0, if I understand it correctly.
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.
That is why it is "waived" - it's a small thing, happy to remove it if you think it is superfluous.
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.
@MicahZoltu @lightclient any comments here?
[Ownable](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/ownership/Ownable.sol). For | ||
brevity, we haven't included any method for assigning or transferring ownership. | ||
|
||
Every time we refer to cTokens, we mean Compound Tokens, as defined in the official |
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.
Hm, I don't see a specification there. What is the interface of a cToken and why does ERC-2212 need to use cTokens and not any ERC-20 ?
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.
Afaik, Compound did not publish the interface for cTokens in this repo, only on their website.
This spec can't use ERC-20, because of the need to calculate interest and withhold a portion of it. Then, I didn't include other protocols other than Compound merely because it would've taken more time and I wanted to get it out there and get feedback.
If you read the latest messages in #2212, you can see that we started working on a v2, which is protocol-agnostic. v1 is not.
The Travis build failed with this error:
Is this a normal failure? |
Please rebase and fix any potential validation errors, if you are still pursuing this. |
68ec16f
to
7c23143
Compare
EIPS/eip-2212.md
Outdated
brevity, we haven't included any method for assigning or transferring ownership. | ||
|
||
Every time we refer to cTokens, we mean Compound Tokens, as defined in the official | ||
[documentation](https://compound.finance/ctokens). These are ERC-20 tokens with an interest accrual feature built in. |
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 link is now dead.
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.
Is it? I just opened it and it worked for me.
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.
It doesn't go to any kind of documentation, just a marketing page. There should be a link to a specification, if it cannot be another ERC.
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.
Fair enough.
SHOULD revert if `staker` doesn't have an active stake. SHOULD revert if `tokenAddress` doesn't match either the cToken or the underlying of the stake. | ||
|
||
```solidity | ||
function balanceOf(address staker, address tokenAddress) external view returns (uint256); |
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.
Since the ABI encoding ignores the parameter names, this may be something prone to collisions.
EIPS/eip-2212.md
Outdated
**Triggers Event**: [DiscardCToken](#discardctoken) | ||
|
||
```solidity | ||
function discardCToken(address cTokenAddress) external |
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.
I think it would be better using the type system if there is a requirement to use "cToken"s.
Please consider my comments about interfaces in #2467. I'm not sure I fully understand the specification, how an implementation would do "staking". Furthermore the lack of documentation about @MicahZoltu do you have any comments? |
Yeah "staking" is not the best wording I could've come up with. The idea is that people deposit stablecoins into a smart contract and the contract starts earning interest on the deposits using cTokens. I agree that cTokens are more like a proprietary product and lack a comprehensive documentation. Since I've stopped promoting this ERC in the meantime, I'm going to close the PR. |
Sorry for the hassle and thanks for being considerate here. Maybe if you do not plan to promote this idea further, it would be useful to leave a similar message on #2212 and/or close it. |
No worries, thanks for your support. I just updated the other thread. |
As per @axic's comment in #2212, I add the markdown version of ERC-2212 so it can be viewed on eips.ethereum.org.