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
Contributing.md #587
Contributing.md #587
Conversation
@TheBlueMatt @Kixunil addressed your comments with a separate commit 4e64db1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor nits, looks good overall. Thx for tackling this issue :)
The only major thing we should talk about more are security disclosures imo.
ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General ACK, just please revert the changes to the dependency versions (was probably just an oversight).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 92bb0e5
@apoelstra did all of that + tried to make CI scripts to match the guidelines as you described |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be updated with the latest IRC channels.
@sanket1729 did that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e1c8e13
@Kixunil pls re-ACK. I updated the file according to Andrew's review. We have removed derive section to be added later as a separate PR after a proper discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e1c8e13
@apoelstra shell we wait for more ACKs here - or this can be merged already? |
Let's wait for one more ACK. It's not going to conflict with anything. |
CONTRIBUTING.md
Outdated
Communication about Rust Bitcoin happens primarily in | ||
[#bitcoin-rust](https://web.libera.chat/?channel=#bitcoin-rust) IRC chat on | ||
[Libera](https://libera.chat/) with the logs available at | ||
<http://gnusha.org/rust-bitcoin/>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks the logs at http://gnusha.org/rust-bitcoin/ didn't switch to libera because they are not working since about June 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We have to contact Bryan Bishop to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll ping him.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just checked and it looks liek https://gnusha.org/bitcoin-rust/ is up to date.
So actually, we should change our CONTRIBUTING file and our IRC /topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I do that with a fixup commit not to make re-review the whole PR for others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just checked and it looks liek https://gnusha.org/bitcoin-rust/ is up to date.
Is it correct that days are there, but they are empty (except from log-start/log-end) up to June 2021?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is very little activity on the channel, but the logs do not appear empty to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I was watching http://gnusha.org/rust-bitcoin/ while you linked http://gnusha.org/bitcoin-rust/ (rust and bitcoin switched)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@apoelstra @Kixunil @RCasatta @sanket1729 I fixed the reference to IRC logs as a separate fixup commit 31c4983. Pls re-ack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 31c4983
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 31c4983
Sorry for disturbing, @RCasatta @sanket1729, but this needs just a review of a single fixup commit in 31c4983 since your last reviews of this PR. We don't want to merge without your ACKs since you did a lot of commenting here before. |
ACK 31c4983 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 31c4983. Looks good
Seems like we have 4 ACKs now (@RCasatta did it in a wrong way not clicking GitHub button, but I checked with bitcoin merge tool, it still sees/uses it). @apoelstra wouldn't you mind about merging it? |
I noticed one should take care using the word ACK in phrases not meant to be for acking otherwise the tool will take it for the "ACKs for top commit:" section even if it's not intended. |
I think, as long as you don't mention the top commit in the same line, you should be okay? |
yes, if you don't mention the top commit you are fine, pretty rare false positive, so it happened this time :) |
1b2dbd7 0.28.0-rc.1 release (Dr Maxim Orlovsky) Pull request description: We still have quite a few issues and PRs pending to be addressed/merged before full 0.28 release: see https://github.com/rust-bitcoin/rust-bitcoin/milestone/10 Most important, we have to find a way to address #777; additionally it will be desirable to get #587, #786, #776. We also have quite of work to write CHANGELOG in #785 Nevertheless I propose to start with a `beta-1` subrelease to unlock ability for `miniscript` release and downstream testing. The Taproot API looks final, and the pending PRs are addresses internal issues/bugs which is perfectly fine for beta releases. ACKs for top commit: Kixunil: ACK 1b2dbd7 Tree-SHA512: a7bd6dd3e7a489f7fd758fb0d7a60103bfe0cdf88997b2163f163841fa1c12e7b31fa8f03b9f817a671379578dbc10a2ca583f49b64d2d2de4a9ebb57b2b9bd5
We've being discussing that a
CONTRIBUTING.md
guidelines are needed for some time and I took an effort to propose it. The main text is taken fromrust-lightning
version (with some additions), but it is extended on the topics we were discussing in multiple issues previously:rustfmt
Files are not formatted with latest rustfmt #172Each of these issue-related amendments to the basic (modified) version of rust-lightning contributing guidelines are made with a separate commit.