Skip to content
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

docs: add reviewers guide #5664

Merged

Conversation

stringhandler
Copy link
Collaborator

Description

Add a reviewer's guide for new contributors

Motivation and Context

To educate new developers and upskill them to be effective reviewers

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@github-actions
Copy link

github-actions bot commented Aug 25, 2023

Test Results (CI)

1 200 tests   1 200 ✔️  53m 39s ⏱️
     38 suites         0 💤
       1 files           0

Results for commit 7d56fb6.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Aug 25, 2023
@github-actions
Copy link

github-actions bot commented Aug 25, 2023

Test Results (Integration tests)

27 tests   27 ✔️  12m 53s ⏱️
11 suites    0 💤
  2 files      0

Results for commit 7d56fb6.

♻️ This comment has been updated with latest results.

sdbondi
sdbondi previously approved these changes Aug 25, 2023
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start!

Some additional ideas:

  • overflowing integer conversions (my_u64 as u32, etc)
  • bounds checking (copy_from_slice etc)
  • attackers use resources without bounds (e.g. can they stream junk forever)
  • unsafe code blocks
  • zeroize private data (w/ things to look out for with stack memory)

docs/src/reviewing_guide.md Show resolved Hide resolved
docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
#### Tokio
Tokio has many useful channels, and it can be difficult to know what to look out for, so here are some things to keep in mind:
1. When using `watch`:
1. Watches can block if the reference returned from `borrow()` is held for a long time. Any call to `borrow()` should drop the reference as soon as possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL :)

docs/src/reviewing_guide.md Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project removed P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Aug 25, 2023
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
stringhandler and others added 2 commits August 25, 2023 15:34
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good addition

docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
docs/src/reviewing_guide.md Show resolved Hide resolved
docs/src/reviewing_guide.md Show resolved Hide resolved
docs/src/reviewing_guide.md Show resolved Hide resolved
docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
docs/src/reviewing_guide.md Outdated Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Aug 25, 2023
stringhandler and others added 2 commits August 28, 2023 09:44
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
@stringhandler stringhandler marked this pull request as draft August 30, 2023 14:48
stringhandler and others added 6 commits September 7, 2023 09:42
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
@stringhandler stringhandler marked this pull request as ready for review September 7, 2023 13:53
@stringhandler stringhandler merged commit 6be6e59 into tari-project:development Sep 19, 2023
14 checks passed
@stringhandler stringhandler deleted the st-reviewers-guide branch September 19, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants