Skip to content

Latest commit

 

History

History
60 lines (52 loc) · 4.12 KB

code_reviews.md

File metadata and controls

60 lines (52 loc) · 4.12 KB

The "Yieldcurity" Standard

Opinionated security, code quality standards, and code review policies and procedures for Solidity smart contracts. Inspired by and fully incorporating The Solcurity Standard.

Code author creates pull request:

  • Include a meaningful description explaining the change - highlight areas of risk or complexity
  • Reference Github issue
  • Include hardhat-gas-reporter report. If updating pre-existing code, include both before and after reports
  • Include report from SMTChecker or other analysis if conducted
  • Assign a priority tag to the PR if it needs to be done faster than 3 days
  • Assign reviewers
  • Schedule a time for video review - add details (time/Zoom link) in PR description
  • Consider inviting persons outside the smart contract team (ie front end) or non devs (marketing etc) when demoing a new feature or important change
  • Notify invitees/reviewers

Code author leads video review session:

  • Record the session
  • Explain context of change
  • Demo the change if possible
  • For bug fixes, explain or demo the broken version vs the fixed version
  • Highlight areas of risk or complexity
  • Explain tests if they help understand the new code changes or if there is particular complexity in the tests themselves, otherwise, leave it to the reviewers to think about a testing approach on their own before reviewing the tests

Reviewers:

  • Reviewers should conduct an independent, thorough review
  • Normally, the review should be completed within 3 days of being notified (faster if priority tagged)
  • Refer to the Solcurity smart contract code quality standards
  • Some general tips:
    • Construct a mental model of what you expect the contracts to look like before checking out the code. Compare the architecture to your mental model. Look into areas that are surprising
    • Look at areas that can do value exchange
    • Look at areas that interface with external contracts and ensure all assumptions about them are valid
    • Create a threat model and make a list of theoretical high level attack vectors
    • Do another review from the perspective of every actor in the threat model
  • Go through the Solcurity standards checklist for each of the following:
  • Consider these additional Yield security precautions:
    • Never ever allow users to call arbitrary contracts from our contracts. Either use auth or some kind of whitelist.
  • After reviewing the code changes, see the Code, Contract, and DeFi checklists
  • Before reviewing tests, construct a mental model of how you would approach testing:
    • Are realistic scenarios/values used in tests?
    • Are all important edge cases and states being tested?
    • For bug fixes, is there a new test that would have caught the bug?
    • If there are any issues that should hold up merge, mark the PR as Changes Requested
    • Approve If completely comfortable with the changes

Code Author obtains final approval and merges PR:

  • Address any comments or changes requested, be sure to mark comments as resolved
  • Notify reviewers if any changes are made to obtain final approval
  • Must have at least 1 approval prior to merge
  • Merge PR and close related Github issue

Edit this page