Skip to content

update bitcoin to 0.31.2 #1300

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

Closed
wants to merge 17 commits into from
Closed

Conversation

getong
Copy link

@getong getong commented Jan 27, 2024

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

wait for

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

Hey, I still haven't looked at the code in depth, but I have a few comments:

  • You are upgrading the project MSRV from 1.63 to 1.70 - that's not a change we take lightly, is there any reason behind it? Has this been discussed with the team?
  • Commits should be atomic and hygienic, and the message should explain what you're doing. You currently have multiple commits doing the same thing, which should be squashed together. I suppose this PR should have one commit updating to rust-bitcoin 0.31 + more commits if you're doing something else (updating the MSRV, for example, would need its own commit). More info here: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches or here: https://blog.lanesawyer.dev/17501/git-hygiene
  • The CI is currently failing, can you look into that please?

@@ -12,7 +12,7 @@ jobs:
rust:
- version: stable
clippy: true
- version: 1.63.0 # MSRV
Copy link
Member

Choose a reason for hiding this comment

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

Why are you updating the project MSRV?

@getong
Copy link
Author

getong commented Jan 30, 2024

I have not finished the upgrade code, some code test are not passed.
Need more time.

@danielabrozzoni
Copy link
Member

No problem! I suggest you keep this PR in draft in the meantime, then :)

@evanlinjin evanlinjin marked this pull request as draft February 2, 2024 01:43
@getong getong closed this Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants