Better codify contribution rules via `CONTRIBUTING.md` and create issue templates

[thebrandonlucas]

It seems there are a few rules for contributing which are not listed in the README which can cause a lot of back-and-forth during new developer's attempts to contribute. Here is what seems to not be explicitly stated FWICT. Might be nice to put this into some explicit CONTRIBUTING.md file or something of the like so that a new contributor can be setup for success with minimal pairing time from the team.

As a potential reference, I think Bitcoin.org does this well/pretty extensively which gave me a lot of confidence in proceeding on my own.

Rules not listed as far as I am aware:

• Fork the repo and make a new branch before working on it (push to your fork then make a PR from that fork, no "direct" PRs)
• Every commit must pass CI (provide example git hook) (not just every PR!). This was non-obvious to me and I think a relatively rare rule (the README uses language like "before submitting any changes" which I think is typically interpreted to mean "before opening a PR"). To do this we must essentially ensure 3 scripts succeed before committing: ./contrib/test.sh (or ./contrib/test_local.sh for only Rust nightly), ./contrib/lint.sh, and cargo fmt --all -- --check. @DanGould has provided a nice example script here for pre-commit hooks. It would probably be worthwhile to have this script written down in a .md file for contributors to reference as a suggested pre-commit hook. Or, we can set that pre-commit hook as a default for this repo and tell developers they can pass --no-verify if they'd like to skip verification.
• If after PR review feedback you need to fix something, amend the commit to do it instead of adding a "fix" commit to keep the commit history clean.

I'm sure I missed some, feel free to add. Essentially anything we can anticipate in advance that would cause us to reject a PR would be useful to write.

It is probably also worthwhile to discuss creating issue templates for the respective sub-projects, which are really helpful to guide the user providing all the relevant information we'd like to have to set us up for successful debugging (cargo version, checklist verifying they ran all the scripts first, suggested use of RUST_LOG=debug for payjoin-cli, etc). nix develop could probably help a lot here, but I believe it is currently broken.

Would love to hear feedback. More than happy to work on this once we're in agreement about how best to do it.

[DanGould]

I think you've got a pretty good sense of what's missing and what to document. Our requirements are far similar to bitcoin's own CONTRIBUTING.md rather than bitcoin.org, which does not offer a security-critical systems software.

Fork the repo and make a new branch before working on it (push to your fork then make a PR from that fork, no "direct" PRs)

This is mentioned in bitcoin's contributing, but I don't think it'll be an issue for most people. You have permissions to push to these repos. The vast majority of poeple do not.

Every commit must pass CI If after PR review feedback you need to fix something, amend the commit to do it instead of adding a "fix" commit to keep the commit history clean.

This isn't a strict rule because we're not going to set up the tooling to enforce it because it's too expensive in terms of taking time to run. Leaving commit hooks in docs is fine though I don't want to burden each contributor with them as mandatory. The reason we do this is so that we can find bugs with git bisect.

Both that pass tests/lints/fmts concern and the 'don't commit fixes, rebase and force push' are hygeine concerns.

Issue templates sound good to me. Keep it simple, and focus on the absolutely necessary changes. I don't think we need consensus about how this kind of thing is done. These kinds of issues are rife for bikeshedding.

[thebrandonlucas]

Our requirements are far similar to bitcoin's own CONTRIBUTING.md rather than bitcoin.org, which does not offer a security-critical systems software.

Thanks for the pointer, I'll model ours after that.

This is mentioned in bitcoin's contributing, but I don't think it'll be an issue for most people. You have permissions to push to these repos. The vast majority of poeple do not.

I agree, but I tend to favor a little verbosity when it makes for friendlier docs.

This isn't a strict rule because we're not going to set up the tooling to enforce it because it's too expensive in terms of taking time to run. Leaving commit hooks in docs is fine though I don't want to burden each contributor with them as mandatory.

Agreed, I wasn't planning on enforcing that, but rather providing docs/easy steps for someone to set it up if they so chose.

The reason we do this is so that we can find bugs with git bisect.

That reasoning might be useful to document somewhere as well.

Both that pass tests/lints/fmts concern and the 'don't commit fixes, rebase and force push' are hygeine concerns.

Noted 👍

Issue templates sound good to me. Keep it simple, and focus on the absolutely necessary changes. I don't think we need consensus about how this kind of thing is done. These kinds of issues are rife for bikeshedding.

Will do, won't spend too many cycles on it. I think a little effort here can go a long way.

Thanks for your thoughts, hugely helpful. This is enough for me to churn out something that will hopefully go a long way for new/returning contributors. Will work on this shortly.