-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Set ERC-1620 to "Last Call" #2467
Conversation
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
|
Hey @axic! Sorry to tag you, I was wondering if this would be a good time to merge the PR? It seems changing the status to "Last Call" requires a moderator approval. |
@PaulRBerg sorry this was totally missed, can you rebase this if it is still valid? |
29574cc
to
6b46062
Compare
No worries, just rebased now. |
8f60135
to
431c9bd
Compare
EIPS/eip-1620.md
Outdated
[RICOs](https://github.com/lukso-network/rico), or Reversible ICOs, were introduced at Devcon4 by @frozeman. The idea is to endow investors with more power and safety guarantees by allowing them to "reverse" the investment based on the evolution of the project. We previously discussed a similar concept called SICOs, or Streamable ICOs, in this research [thread](https://ethresear.ch/t/chronos-a-quirky-application-proposal-for-plasma/2928/14?u=paulrberg). | ||
|
||
Instead of investing a lump sum and giving the money away to the project developers, funds are held in a smart contract which allocates money based on the passage of time. Project developers can withdraw funds as the stream stays active, while investors have the power to get back a significant percentage of their initial commitment if the project halts. | ||
[Sablier](https://sablier.finance) is a champion project for this ERC. It is what we call the protocol for real-time finance. |
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'm not sure this paragraph is relevant here, but will defer to @MicahZoltu's judgement.
EIPS/eip-1620.md
Outdated
|
||
```solidity | ||
function create(address _recipient, address _tokenAddress, uint256 _startBlock, uint256 _stopBlock, uint256 _payment, uint256 _interval) | ||
function getStream(uint256 streamId) view returns (address sender, address recipient, uint256 deposit, address token, uint256 startTime, uint256 stopTime, uint256 remainingBalance, uint256 ratePerSecond) |
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.
If you are using Solidity syntax here, then I suggest to perhaps consider formatting it according to the style guide for readability, but at least add a semicolon to the end, so it could be copy pasted into an interface.
Furthermore some ERCs chose to describe it as a Solidity interface. Some also chose to define their ERC-165 identifiers.
This is just a comment you may or may not want to take on board and can be addressed here or later.
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 is fine to go ahead, hoping there will be some feedback from reviewers. I'd ask @MicahZoltu for brief feedback, otherwise I think we should merge this tomorrow.
Hey @axic, I applied your recommendations, except for this:
How would you rewrite the functions in this PR so that they follow the style guide? As with regard to ERC-165, I'm not particularly familiar with it and I'd be happy to not include it in ERC-1620 for now. |
I mean that they are formatted like this: function getStream(uint256 streamId)
view
returns (address sender, address recipient, uint256 deposit, address token, uint256 startTime, uint256 stopTime, uint256 remainingBalance, uint256 ratePerSecond); Which makes is easier to read. I think the style guide has more recommendations. I'd suggest to check out some ERCs which define a Solidity interface upfront and explain the functions, either as you do now, or via NatSpec.
See ERC-165 which basically means support of an interface can be determined via checking for the interface id. And defining a Solidity interface makes this very apparent to the reader too. |
Thanks for your suggestions! I look forward to improving the formatting and the interface before the ERC is moved to "Final". But in the meantime, can we move it to "Last Call"? I've been waiting for this for a while haha. |
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 is unclear to me why this needs to be a standard rather than just a "good idea". An interface standard is useful when you have a many-to-many relationship like browsers (many) to websites (many) and when you don't have a fixed set of either one up front (new browsers come and go, new websites come and go). In this case, each stream arrangement will involve exactly 2 parties, and those parties can agree on using whatever interface/tools they want. It is unclear why we need a global standard that all money streams need to follow. Can instead people just use some application like Sablier?
The specification also feels unnecessarily large in the case where we want standardized tooling (e.g., wallets) to support current stream balances. It seems like a simple:
interface MoneyStream {
function balanceOf(address user) returns (uint256 amount);
}
would be sufficient so users can see their current balance of a given money stream. Perhaps one more function for getting the details of the stream.
A potentially better pattern here may be to create a new contract per stream, rather than having a single contract that encapsulates multiple streams. Perhaps in the rationale section you could describe why multiple streams per contract is valuable or necessary?
--- | ||
|
||
## Simple Summary | ||
Money streaming represents the idea of continuous payments over a finite period of time. Block numbers are used as a proxy of time to continuously update balances. | ||
|
||
We define money streaming as continuous payments over a finite period of time. Block timestamps are used as an on-chain proxy for time. |
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.
Block timestamps are used as an on-chain proxy for time.
This seems like an implementation detail, which is not appropriate for a standardized interface. If we remove that, we are left with:
We define money streaming as continuous payments over a finite period of time
Which seems like the definition of a new phrase "money streaming" and an EIP isn't the right place to try to define new terminology.
Consider rewriting this to more clearly define what this is trying to standardize. If you are just trying to standardize a phrase, then I recommend closing this and pursuing that elsewhere.
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.
Well, they have code, so it's not just terminology. Should be something like, "Standard for sending continuous payments over time."
|
||
## Motivation | ||
This standardised interface aims to change the way we think about long-term financial commitments. Thanks to blockchains, payments need not be sent in chunks (e.g. monthly salaries), as there is much less overhead in paying-as-you-go. Money as a function of time would better align incentives in a host of scenarios. | ||
|
||
This standardised interface takes a stab at changing the way we think about trust in financial contracts. Thanks to blockchains, payments need not be sent in lumps (as with monthly salaries), as there is less overhead in pay-as-you-go. Money as a function of time would better align incentives in a host of scenarios. |
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.
Recommend phrasing this from the point of view of a final EIP. This means that this isn't "taking a stab at" anything, but instead it is strongly defining something.
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.
+1. If it is "taking a stab at" this implies the EIP is not functional. Should be rephrased.
[RICOs](https://github.com/lukso-network/rico), or Reversible ICOs, were introduced at Devcon4 by @frozeman. The idea is to endow investors with more power and safety guarantees by allowing them to "reverse" the investment based on the evolution of the project. We previously discussed a similar concept called SICOs, or Streamable ICOs, in this research [thread](https://ethresear.ch/t/chronos-a-quirky-application-proposal-for-plasma/2928/14?u=paulrberg). | ||
|
||
Instead of investing a lump sum and giving the money away to the project developers, funds are held in a smart contract which allocates money based on the passage of time. Project developers can withdraw funds as the stream stays active, while investors have the power to get back a significant percentage of their initial commitment if the project halts. | ||
[Sablier](https://sablier.finance) is a champion project for this 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.
[Sablier](https://sablier.finance) is a champion project for this ERC. |
EIPs shouldn't be platforms for advertising specific products. The list above is reasonable, and more clearly defines for users what this EIP may be useful 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.
+1
- Salaries | ||
- Insurance | ||
- Subscriptions | ||
- Consultancies | ||
- CDPs | ||
- Rent | ||
- Parking | ||
- Lending |
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.
Consider a comma separated list rather than a bulleted list as each of these are one-word long so we don't really need the strong separation between them.
## Additional References | ||
|
||
- [Sablier Launch Thread](https://twitter.com/SablierHQ/status/1205533344886411264) | ||
- [Introducing Sablier: Continuous Salaries on Ethereum](https://medium.com/sablier/introducing-sablier-continuous-payments-on-ethereum-c2bf04446d31) | ||
- [Chronos Protocol Ethresear.ch Plasma Proposal](https://ethresear.ch/t/chronos-a-quirky-application-proposal-for-plasma/2928?u=paulrberg) | ||
- [Chronos Protocol White Paper](http://chronosprotocol.org/chronos-white-paper.pdf) | ||
- [Flipper: Streaming Salaries @ CryptoLife Hackathon](https://devpost.com/software/flipper-3gvl4b) | ||
- [SICOs or Streamed ICOs](https://ethresear.ch/t/chronos-a-quirky-application-proposal-for-plasma/2928/14?u=paulrberg) | ||
- [RICOs or Reversible ICOs](https://twitter.com/feindura/status/1058057076306518017) | ||
- [Andreas Antonopoulos' Keynote on Bitcoin, Lightning and Money Streaming](https://www.youtube.com/watch?v=gF_ZQ_eijPs) |
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 isn't an official section, and while unofficial sections are not strictly prohibited we try to avoid them in favor of the standardized sections. Keeping in mind that EIPs are technical specifications, and not meant as a store of history description of use cases, can we just delete this whole section?
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.
Marketing to products in EIPs should be removed. Having references in themselves isn't too bad, especially to avoid plagiarism.
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. |
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. |
Hey everyone, sorry for not replying here earlier. I read @MicahZoltu's suggestions, started applying them, and then I realised that what's left is not much. Micah is right. The specification and its potential reach as a standard are not ample enough to warrant an ERC. |
@MicahZoltu @axic will this EIP be moved to |
@edsonayllon Ah, this should be moved to |
Can confirm that I'm okay with this being withdrawn. Thanks for the shout-out, Micah! |
As per the announcement in #1620, update the markdown version of ERC-1620 to its final version from Sep 27, 2019.