First draft, don't-cover-everything-but-yet-a-good-start-CONTRIBUTING.md #470

ariard · 2020-01-31T16:43:12Z

A CONTRIBUTING.md, a code formatter, mutation testing. We just have to run Travis on SPARC-32 and we have a real open source project.

Close #468

moneyball · 2020-01-31T19:39:02Z

@ariard thanks for taking a first pass at this. I did a quick initial review:

I like the overall tone and sentiment
We're setting up a Slack for discussion of LDK and Rust-Lightning so once that is completed and public we'll want to mention it here
There are some grammatical fixes but can wait until we have more feedback from others on content.

jkczyz · 2020-01-31T22:24:44Z

CONTRIBUTING.md

+Deeply tied with the security aspect, Rust-Lightning developers take testing
+really seriouslt. Due to the modular nature of the project writing new functional
+tests is easy and well-coverage of the codebase is a long-term goal.


I'd also like to see more unit testing in order to promote less complex, modular design.

Yes add a line on it

TheBlueMatt · 2020-02-03T02:34:03Z

CONTRIBUTING.md

+
+In general commits should be atomic and diffs should be easy to read.
+For this reason do not mix ant formatting fixes or code moves with
+actual code changes.


Further, each commit, individually, should compile and pass tests, in order to ensure git bisect and other automated tools function properly.

Can you add this line?

TheBlueMatt · 2020-02-03T02:34:48Z

CONTRIBUTING.md

+Architecture
+------------
+
+XXX: (here or in readme ?)


Probably readme, at least in general, though specific "fuzzers live in fuzz/src, with message fuzers in ..." kind of stuff could go here.

TheBlueMatt · 2020-02-03T02:35:39Z

CONTRIBUTING.md

+helps prevent user loss of funds. If you think vulnerability is with regard to the spec
+please inform other Lightning implementations diligently.
+
+XXX: (what process ?)


Pre-0.1 we can probably say "open github issue". Post-0.1 we should have some list of PGP keys and suggest emailing (to something not on gmail).

TheBlueMatt · 2020-02-03T02:36:19Z

Cool! I left a few high-level comments. Indeed, we'll need someone to go over it for grammar, but I'm probably not the one to do that either lol.

TheBlueMatt · 2020-02-03T02:40:37Z

Also, we should add a line explaining that we use Travis to enforce MSRV of Rust 1.22.0 for the lightning/ workspace (though not for lightning-net-tokio or fuzzers).

moneyball · 2020-02-03T20:27:45Z

Do we want to state "we dont make syscalls" somewhere in the README or CONTRIBUTING doc or elsewhere, so that new contributors understand this important principle of the design?

maxgiraldo · 2020-02-03T21:06:58Z

Do we want to state "we dont make syscalls" somewhere in the README or CONTRIBUTING doc or elsewhere, so that new contributors understand this important principle of the design?

I think this should live in the README under something like ## Design Goals. Or if you want to keep the README high level, then say something like "We make no assumptions about the underlying platform"

maxgiraldo

I would also suggest adopting some sort of commit-prefixing protocol, e.g. conventional commits or similar variant (like what bitcoin core uses). This can help delineate the type of change being made.

jkczyz · 2020-02-03T21:45:19Z

I would also suggest adopting some sort of commit-prefixing protocol, e.g. conventional commits or similar variant (like what bitcoin core uses). This can help delineate the type of change being made.

Speaking of commits, I'd like to see some guidelines around both commit messages and PR descriptions. When exploring the git log recently, I noticed the why for many commits was not given, which makes it difficult to understand the rationale for a change. Sometimes I was able to ascertain this information from the PR description, but often not.

Having clear guidelines about what goes into a commit message and PR description would benefit posterity, including our future selves.

maxgiraldo · 2020-02-03T22:04:35Z

Having clear guidelines about what goes into a commit message and PR description would benefit posterity, including our future selves.

Linus has a solid guideline.

ariard · 2020-02-10T02:32:33Z

Thanks all for your review, updated with your comments.

@moneyball sorry miss #479, you mention LDK but don't link to any other thinh so take opportunity of this PR to add a link to LDK slack in README.md

TheBlueMatt

LGTM, nitted the grammar here and there.

lightning/src/ln/channelmanager.rs

CONTRIBUTING.md

ariard · 2020-02-11T02:17:55Z

Thanks for review, addressed your comments Matt!

jkczyz · 2020-02-11T22:15:30Z

Having clear guidelines about what goes into a commit message and PR description would benefit posterity, including our future selves.

Linus has a solid guideline.

Thanks for passing this along! Some of that is a bit project specific, though. Could we possibly use the same guidelines as Bitcoin Core:

https://chris.beams.io/posts/git-commit/

I'd be up for considering conventional commits (your other suggestion), also. I haven't put much thought into it yet. Do others have experience with them?

maxgiraldo · 2020-02-11T22:22:21Z

I'd be up for considering conventional commits (your other suggestion), also. I haven't put much thought into it yet. Do others have experience with them?

We're piloting a variant of conventional commits at my work. Got some pushback from other developers because it requires some additional work on their part and it's hard to prove the value it provides. Because of this, I think using the same or similar guidelines as Bitcoin Core would be the most beneficial.

CONTRIBUTING.md

lightning/src/ln/channelmanager.rs

jkczyz · 2020-02-11T22:24:50Z

CONTRIBUTING.md

+Anyone may participate in peer review which is expressed by comments in the pull
+request. Typically reviewers will review the code for obvious errors, as well as
+test out the patch set and opine on the technical merits of the patch. PR should
+be reviewed first on the conceptual level before focusing on code style or grammar
+fixes.


Would it be worth stating the criteria used for merging? Or leave to a follow up?

Good question, @TheBlueMatt should we enforce the at-least 2-ACK for any substantial changes ?

I was kinda figuring leave it as a followup - we just dumped 4 new full-time contributors on the project, so I was kinda waiting until folks were up to speed and felt more comfortable with the codebase then we'd revisit it and do something like 2 acks from regular contributors or so (like rust-bitcoin).

ariard · 2020-02-11T23:57:12Z

Updated 0dedb38 with last comments fixed, specially referring to Bitcoin Core git commit convention instead of Linus one.

TheBlueMatt

Aside from the two comments, looks good.

ariard · 2020-02-12T18:35:46Z

Two comments fixed at 34871ea

jkczyz reviewed Jan 31, 2020

View reviewed changes

ariard force-pushed the 2020-01-contributing-draft branch from 1818a30 to 15df6b1 Compare February 3, 2020 00:38

TheBlueMatt reviewed Feb 3, 2020

View reviewed changes

maxgiraldo reviewed Feb 3, 2020

View reviewed changes

ariard force-pushed the 2020-01-contributing-draft branch from 15df6b1 to 49bfed9 Compare February 10, 2020 02:29

TheBlueMatt reviewed Feb 10, 2020

View reviewed changes

ariard force-pushed the 2020-01-contributing-draft branch from 49bfed9 to d16ee5e Compare February 11, 2020 02:17

jkczyz reviewed Feb 11, 2020

View reviewed changes

ariard force-pushed the 2020-01-contributing-draft branch from d16ee5e to 0dedb38 Compare February 11, 2020 23:56

TheBlueMatt reviewed Feb 12, 2020

View reviewed changes

Add a first draft CONTRIBUTING.md

34871ea

ariard force-pushed the 2020-01-contributing-draft branch from 0dedb38 to 34871ea Compare February 12, 2020 18:34

TheBlueMatt approved these changes Feb 12, 2020

View reviewed changes

TheBlueMatt merged commit bae2338 into lightningdevkit:master Feb 12, 2020

First draft, don't-cover-everything-but-yet-a-good-start-CONTRIBUTING.md #470

First draft, don't-cover-everything-but-yet-a-good-start-CONTRIBUTING.md #470

Uh oh!

Conversation

ariard commented Jan 31, 2020

Uh oh!

moneyball commented Jan 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 3, 2020

Uh oh!

TheBlueMatt commented Feb 3, 2020

Uh oh!

moneyball commented Feb 3, 2020

Uh oh!

maxgiraldo commented Feb 3, 2020

Uh oh!

maxgiraldo left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Feb 3, 2020

Uh oh!

maxgiraldo commented Feb 3, 2020

Uh oh!

ariard commented Feb 10, 2020

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard commented Feb 11, 2020

Uh oh!

jkczyz commented Feb 11, 2020

Uh oh!

maxgiraldo commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Feb 11, 2020

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Feb 12, 2020

Uh oh!

Uh oh!