Contract documentation: providing detailed non-developer description of major functions #23
Conversation
giladHaimov
changed the title
There was a problem hiding this comment.
Great work @giladHaimov! Your attention to detail has revealed several potential improvements and some potential issues. I'm hoping that we can use this PR as the discussion center to align on blockchain codebase documentation standards @jackcrypto9527 @charles2023wood @zfliex924.
My requested revisions can be categorized by:
- Minor grammatical changes (majority of comments)
- The format of our new code documentation standard
- The implications of logic changes, specifically around the additional function modifiers e.g.
CandidateHub.sol - Thoughts on the various potential issues you've identified
Can make the grammatical change revisions, but let's hear from other reviewers regarding revisions from the other categories before making edits.
contracts/BtcLightClient.sol
Outdated
| /* @product Claim relayer rewards | ||
| @param relayerAddr The relayer address | ||
| @logic Called by relayers to claim their rewards for storing BTC blocks | ||
| - Reverts if the provided address is not of a relayer or if the relayer currently has no reward to claim |
There was a problem hiding this comment.
| - Reverts if the provided address is not of a relayer or if the relayer currently has no reward to claim | |
| - Reverts if the provided address is not a relayer or if the relayer currently has no reward to claim |
contracts/BtcLightClient.sol
Outdated
| @logic Called by relayers to claim their rewards for storing BTC blocks | ||
| - Reverts if the provided address is not of a relayer or if the relayer currently has no reward to claim | ||
| - Else: invokes SystemReward's claimRewards() to transfer the reward to the relayer. | ||
| Note that claimReward() may slash the reward to the current SystemReward balance if the latter is smaller than the reward without revertingthe Tx |
There was a problem hiding this comment.
| Note that claimReward() may slash the reward to the current SystemReward balance if the latter is smaller than the reward without revertingthe Tx | |
| Note that claimReward() may slash the reward to the current SystemReward balance if the latter is smaller than the reward without reverting the Tx |
| - Else: invokes SystemReward's claimRewards() to transfer the reward to the relayer. | ||
| Note that claimReward() may slash the reward to the current SystemReward balance if the latter is smaller than the reward without revertingthe Tx | ||
| Note2: this function may be called by any party (not only a relayer) with a relayer address to which the funds will be transferred | ||
| this might prove problematic for relayers that prefer to choose their own dates of reward reclaiming |
There was a problem hiding this comment.
Good call out - relayers may want to control reward dates for tax purposes. It is nice however that the current approach enables user-friendly reward-claiming processes, but I think we could probably achieve similar functionality even with the proposed change.
There was a problem hiding this comment.
I'm adding a new @OpenIssue tag to denote items, as this one, that might require additional R&D/product input at minimum to make sure they're intentional
contracts/BtcLightClient.sol
Outdated
| /// @return The relayer weight | ||
| /* @product Calculate relayer weight based number of BTC blocks relayed | ||
| @param count The number of BTC blocks relayed by a specific validator | ||
|
|
There was a problem hiding this comment.
Any reason for the new line here?
| @logic | ||
| 1. if the total contract balance (i.e. the sum of all burned tokens) exceeds | ||
| the global burnCap then the excess (up to the limit of the current sum of | ||
| tokens to burn) is returned back to the caller |
There was a problem hiding this comment.
Minor: happy to do . or no periods in the descriptions but should aim to be consistent
| tokens to burn) is returned back to the caller | |
| tokens to burn) is returned back to the caller. |
contracts/ValidatorSet.sol
Outdated
| /// @param felonyRound The number of rounds to jail | ||
| /// @param felonyDeposit The amount of deposits to slash | ||
| function felony(address validator, uint256 felonyRound, uint256 felonyDeposit) external override onlySlash { | ||
| /* @product Called by the Slash contract (only) to Slash the validator for felony behaviors |
There was a problem hiding this comment.
| /* @product Called by the Slash contract (only) to Slash the validator for felony behaviors | |
| /* @product Called by the Slash contract (only) to slash validators for felony behaviors |
contracts/ValidatorSet.sol
Outdated
|
|
||
| @logic | ||
| 1. If the 'bad' validator is the only validator then he will not be jailed, but | ||
| its income will be zeroed |
There was a problem hiding this comment.
| its income will be zeroed | |
| his income will be zeroed |
contracts/ValidatorSet.sol
Outdated
| 1. If the 'bad' validator is the only validator then he will not be jailed, but | ||
| its income will be zeroed | ||
| 2. Else the validator will be removed from the current.validators list | ||
| 3. And a sum equal to the 'bad' validator income qill be equaly divided between the |
There was a problem hiding this comment.
| 3. And a sum equal to the 'bad' validator income qill be equaly divided between the | |
| 3. And a sum equal to the 'bad' validator income will be equally divided between the |
contracts/ValidatorSet.sol
Outdated
| its income will be zeroed | ||
| 2. Else the validator will be removed from the current.validators list | ||
| 3. And a sum equal to the 'bad' validator income qill be equaly divided between the | ||
| rest of the validators WITHOUT clearing of the 'bad' validator's income! |
There was a problem hiding this comment.
Good find - potential issue here. Tagging @jackcrypto9527 for further discussions
contracts/ValidatorSet.sol
Outdated
| 2. Else the validator will be removed from the current.validators list | ||
| 3. And a sum equal to the 'bad' validator income qill be equaly divided between the | ||
| rest of the validators WITHOUT clearing of the 'bad' validator's income! | ||
| 4. Finally the CandidateHub;s jailValidator() function will be invoked to place the |
There was a problem hiding this comment.
| 4. Finally the CandidateHub;s jailValidator() function will be invoked to place the | |
| 4. Finally the CandidateHub's jailValidator() function will be invoked to place the |
|
Closed and replaced by PR #26 : |
This PR aims to increase the visibility of the DAO's non-developers into the smart contract. Such visibility is crucial for aligning the entire org into a single, clear, world-image of what the contracts are actually doing. It should also have a major effect on the depth and quality of our auditing process (both internal and external).
Most of the work here involved adding function header comments, in some cases I've also added modifiers where I thought they would result in clearer function signature.
In creating the internal docs I did my best to catch the key functions and to describe what the code does rather than what it is, Of course, this is the start of this effort and we will probably need several review/improvement cycles before the internal docs will have the impact I believe it could have on the org-wise clarity of how the contracts actually behave.
I have used @Product and @logic tags to denote, respectively, high-level comments aimed (also) to the DAO's PMs and the function's flow, or logic, descriptor. Feel free to suggest better taggings