This repository has been archived by the owner on Jul 19, 2019. It is now read-only.
Adds inline documentation for the contracts #13
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facuspagnuolo
added
kind:chore
|
@martriay there are some |
|
Yes! Currently on it
|
facuspagnuolo
approved these changes
Apr 12, 2018
There was a problem hiding this comment.
Thanks @martriay ! Looks really good, left some minor comments :)
| @@ -11,23 +11,49 @@ import "zos-core/contracts/upgradeability/UpgradeabilityProxyFactory.sol"; | |||
| import "zos-core/contracts/ImplementationProvider.sol"; | |||
| import "zeppelin-solidity/contracts/math/SafeMath.sol"; | |||
|
|
|||
| /** | |||
| * @title ZepCore | |||
| * @dev This contract controls the kernel distributions and versions for ZeppelinOS | |||
There was a problem hiding this comment.
what about this contract controls the registry and vouching mechanism of the kernel distributions and versions for ZeppelinOS?
There was a problem hiding this comment.
I think the vouching mechanism corresponds to KernelStakes, and that this mechanism is an implementation detail of the version management. ZepCore's role and functionality would not change if instead of staking we were manually selecting the versions.
What do you think?
There was a problem hiding this comment.
yea but actually this one exposes an API for it too
contracts/KernelStakes.sol
Outdated
| * @param staker representing the staker address | ||
| * @param instance representing the kernel instance being unstaked for | ||
| * @param amount representing the amount being unstaked | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) |
contracts/KernelStakes.sol
Outdated
| * @param amount representing the staked amount | ||
| * @param total representing the new total amount staked | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) | ||
| */ |
There was a problem hiding this comment.
I would remove the erc900 part given we are actually not following that standard
There was a problem hiding this comment.
If that's the case, why not remove the data parameter along with it?
contracts/KernelStakes.sol
Outdated
| * @param staker representing the address of the staker | ||
| * @param instance representing the kernel staked for | ||
| * @param amount representing the staked amount | ||
| * @param total representing the new total amount staked |
There was a problem hiding this comment.
I would add for the given address just to be clear
contracts/KernelStakes.sol
Outdated
| * @param instance representing the kernel unstaked for | ||
| * @param amount representing the unstaked amount | ||
| * @param total representing the new total amount staked | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) |
contracts/KernelStakes.sol
Outdated
| * @param staker representing the staker address | ||
| * @param instance representing the kernel instance being staked for | ||
| * @param amount representing the amount being staked | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) |
contracts/ZepCore.sol
Outdated
| * @param developerFraction_ representing the fraction of stakes rewarded to the developer of a kernel instance | ||
| * @param _owner representing the address of the owner | ||
| * @param _owner representing the address of the owner | ||
| * @param _owner representing the address of the owner |
There was a problem hiding this comment.
And that kids is why reviewing is so important
contracts/ZepCore.sol
Outdated
| * @dev Unstakes a given amount for a given kernel instance | ||
| * @param instance representing the kernel instance being unstaked for | ||
| * @param amount representing the amount being unstaked | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) |
contracts/ZepCore.sol
Outdated
| * @param from representing the kernel instance being unstaked for | ||
| * @param to representing the kernel instance being staked for | ||
| * @param amount representing the amount of stakes being transferred | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) |
contracts/ZepCore.sol
Outdated
| * @param staker representing the address of the staker | ||
| * @param instance representing the kernel instance being staked for | ||
| * @param amount representing the amount being staked | ||
| * @param data representing additional information for complex staking models. Included to comply with the ERC900 staking interface (https://github.com/ethereum/EIPs/pull/910) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves #12. I left the ZepToken contract out since it's pretty straightforward and we talked about abstracting the vouching token from the kernel.
Before merging, I would like for the reviewer to pay attention to the description of the
dataargument in the staking functions, especially the one fromZepCore'stransferStakewhich is not defined in the ERC900 spec thus is not required for compliance.