CODEX-MARKETPLACE: RFC Feedback #7
Conversation
There was a problem hiding this comment.
Some comments that needs to be fixed.
I noticed one thing. We use the phrase "client node(s)" - eq. the role name with the term "node", but for SPs we often just leave it as "SP", which imho is not nice. I guess this was introduced by Marcin and I have missed it. Could you please go over the spec and fix "SP" to "SP node(s)"?
specs/marketplace.md
Outdated
| 4. The SP MUST call the `fillSlot()` smart contract function with the same parameters and collateral allowance as described in the [Filling Slots](#filling-slot) section. | ||
| 4. The SP MUST call the `fillSlot()` smart contract function with the same parameters and collateral allowance as described in the [Filling Slots](#filling-slots) section. | ||
|
|
||
| Below is a diagram depicting the proving and repair process: |
There was a problem hiding this comment.
That is not true. This is the whole "flow diagram" of Request's states.
There was a problem hiding this comment.
I am not sure about this move. I don't think this is place for the diagram. I guess I would just leave it where it was. Or what is the motivation for it?
There was a problem hiding this comment.
It seems like the two diagrams explain the storage request process a little different. The one I moved mentions the repair process, so I thought it would be better to the repair section. I moved it again, I believe it also could be added to the filling slots section.
There was a problem hiding this comment.
Hmm, can you then move it directly to the "Storage Provider Role" section instead of "Filling slot" section as it is not related only to filling slots but all other parts as well?
| | `nonce` | `byte32` | Random value to differentiate from other requests of same parameters. It SHOULD be a random byte array. | | ||
| | `reward` | `uint256` | Amount of tokens that will be awarded to SPs for finishing the storage request. It MUST be an amount of Tokens offered per slot per second. The Ethereum address that submits the `requestStorage()` transaction MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least an equivalent amount in Tokens. | |
There was a problem hiding this comment.
We wanted to capitalize Tokens like this to signal that it relates to the "Token" defined in the "Definition" section. Or do you think the link will be obvious even if we do it lowercase?
There was a problem hiding this comment.
This makes sense, because of the defined term in definition. But I do not believe the term should be capitalized, unless we mention the term token(for a non ERC-20 token, e.g. ERC-721) within the spec. So the term could be lowercase within the definition section since only one type of token is discussed.
There was a problem hiding this comment.
I agree this should be capitalised because "Tokens" is a proper noun in this case. We are using it in lieu of "CDX" or "Codex tokens" since this is not legally approved, and may change in the future.
There was a problem hiding this comment.
So you are saying keep it uppercased? Then lets revert the lower-casing of the Definition table as well?
There was a problem hiding this comment.
Hmm, I really don't know what the orthodoxy is with a definitions table in a spec. My comment refers to the capitalization of the word "Tokens" in a sentence. In which case, it is generally (but not always) a proper noun.
There was a problem hiding this comment.
I asked Jimmy to lowercase all the "definitions" in the Definition table because of lowercasing of the "Token": https://github.com/codex-storage/codex-spec/pull/7/files#r1785680631
|
|
||
| The Ethereum address of the SP node from which the transaction originates MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least the amount of Tokens required as collateral for the request. | ||
| The Ethereum address, of the SP node from which the transaction originates, MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least the amount of required tokens. |
There was a problem hiding this comment.
| The Ethereum address, of the SP node from which the transaction originates, MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least the amount of required tokens. | |
| The Ethereum address, of the SP node from which the transaction originates, MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least the amount of required tokens for the request's collateral. |
Please keep the note that the tokens are for collateral.
Updated "SP" terms to "SP node" and "SPs" to "SP nodes" except for within the diagram. Also moved definitions back in Semantics section. |
There was a problem hiding this comment.
Jimmy, I have made earlier few suggestions for you to include in the PR. Some of them are even blockers (like the collateral parameter). It is great that I guess you "agree" with them (eq. giving them thumbs-up) but then why you didn't include them in the PR?! Why then are you asking for re-review if they are not included? I mean I will "Request changes" again and will have to wait again for you to incorporate those changes and doing another "re-review" afterward...
Also, please fix the conflicts with master.
|
|
||
| The Ethereum address of the SP node from which the transaction originates MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least the amount of Tokens required as collateral for the request. | ||
| The Ethereum address, of the SP node from which the transaction originates, MUST have [approval](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-) for the transfer of at least the amount of required tokens. |
specs/marketplace.md
Outdated
| 4. The SP MUST call the `fillSlot()` smart contract function with the same parameters and collateral allowance as described in the [Filling Slots](#filling-slot) section. | ||
| 4. The SP MUST call the `fillSlot()` smart contract function with the same parameters and collateral allowance as described in the [Filling Slots](#filling-slots) section. | ||
|
|
||
| Below is a diagram depicting the proving and repair process: |
There was a problem hiding this comment.
Hmm, can you then move it directly to the "Storage Provider Role" section instead of "Filling slot" section as it is not related only to filling slots but all other parts as well?
There was a problem hiding this comment.
There were a few comments where the conversation was marked as "resolved", but the request was not actually resolved. I went through and marked the unresolved comments as "unresolved".
Also, the description of changes in this PR is lacking details. Could you please update it to list the changes that were made, please?
Hmm I think most of those are actually resolved except one. |
No problem. I didn't know there were upstream changes. |
Made should changes to the marketplace RFC.