Move script types to `primitives` by tcharding · Pull Request #3431 · rust-bitcoin/rust-bitcoin

tcharding · 2024-10-01T03:34:54Z

First patch removes bytes_to_asm_fmt as requested by Kix here: #3194 (comment)

Second patch does the move. The move is minimal but there is quite a bit of code moved in script/mod.rs - I believe it is as minimal as required as well.

github-actions · 2024-10-01T03:36:15Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2024-10-01T03:49:43Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2024-10-01T04:19:37Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

jamillambert

As far as I can tell both the inline of bytes_to_asm_fmt and the move to primitives are good. I checked locally that the large sections of code that were moved were unchanged, except for the necessary changes in use statements etc. and all tests pass.
Although I didn’t verify or know how to verify if everything that needed to be moved, and only those items, have been moved.

apoelstra · 2024-10-01T19:05:37Z

bitcoin/src/blockdata/script/borrowed.rs

In 84af64671aa72cc39f5bf67546496d827469601b:

It's fine to unwrap here but the text in the expect message makes no sense. The reason is that "the fmt::Display impl for Self does not return an error". It has nothing to do with memory or writers.

Also, the commit message says you don't know why this method used to exist but you think it is from when Script was Sized. It has nothing to do with Sized. It is because the Display impl used to add a Script(...) wrapper to its output and users (Steven) wanted to get the text out without that noise.

Oh I thought the Write trait has an error for I/O failures but writes to a String (eg writing to memory) never fail.

Thanks for clarifying the other thing, will fix.

I'd like to clear this up because if I see this line in code write!(&mut buf, "{}", self).unwrap(); I have to ask myself "why is it ok to unwrap?", so I think using expect("exactly correct reason why this won't panic") is better.

In this case, probably the best thing to write is "if this returns an error, to_string will panic, which would be a bug in its own right".

Basically, I don't think you're really justified in having this unwrap here...but the implementation of ToString on T: Display in core has the same unwrap, and so everybody who implements fmt::Display has responded by bending over backward to never actually return a fmt::Result::Err, so we're allowed to pretend like the format functions are infallible.

Cheers, turns out I'm a wombat anyways, we can just call self.to_string() and bypass the whole issue - face palm.

apoelstra · 2024-10-01T19:07:46Z

5758abf58e29d8141482d783bceff0bf1f9c4d3a looks good other than the one comment.

tcharding · 2024-10-01T21:29:57Z

Fixed the commit message but did not change the expect message.

github-actions · 2024-10-01T21:30:57Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2024-10-01T22:23:37Z

I checked locally that the large sections of code that were moved were unchanged

Hey @jamillambert I recently learned of some new git configuration options that make reviewing codes moves like this easier. It makes patches be colorized so that one doesn't have to manually check that large sections of code are unchanged.

Add this to your ~/.gitconfig file


[diff]
	colorMoved = zebra
        colorMovedWs = allow-indentation-change

github-actions · 2024-10-10T00:09:32Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2024-10-10T00:30:18Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2024-10-13T16:58:44Z

Face palm, its 4am here and I got out of bed thinking that I really should get this PR done, only to find I did it 4 days ago.

EDIT: Its #3406 that got me out of bed, and it does still need doing :) My review request stand still though please.

Review please @apoelstra.

apoelstra · 2024-10-13T21:31:01Z

Source commit: 4894aadd93b48349b511609311faea6acd81ab6e
Source: /nix/store/j2c1c8r4zfq46c673jw2cqjrf6ik34s3-source
unpacking source archive /nix/store/kr6vgwlly12xkxjp2ikj43vr928ga9z5-source
source root is source
Running phase: patchPhase
@nix { "action": "setPhase", "phase": "patchPhase" }
Running phase: updateAutotoolsGnuConfigScriptsPhase
@nix { "action": "setPhase", "phase": "updateAutotoolsGnuConfigScriptsPhase" }
Running phase: configurePhase
@nix { "action": "setPhase", "phase": "configurePhase" }
Running cd .
Running phase: buildPhase
@nix { "action": "setPhase", "phase": "buildPhase" }
Building src/lib.rs (bitcoin)
Running rustc --crate-name bitcoin src/lib.rs --out-dir target/lib -L dependency=target/deps --cap-lints allow -L native=/nix/store/np4jc4v8lq73r4laf1v92dddx6z2vvg2-rust_secp256k1-sys-0.10.0-lib/lib/secp256k1-sys.out -C debu>
error[E0599]: no method named `to_string` found for reference `&borrowed::Script` in the current scope
   --> src/blockdata/script/borrowed.rs:491:50
    |
491 |         fn to_asm_string(&self) -> String { self.to_string() }
    |                                                  ^^^^^^^^^ method not found in `&borrowed::Script`
    |
    = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
    |
3   | use crate::alloc::string::ToString;
    |

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.

Sorry. I probably ran this several days ago and forgot to dig into and repost the failure.

This function does not need to exist anymore because it is exactly the same as what is produced by the `Display` impl.

Move the `Script` and `ScriptBuf` types to `primitives`, nothing else.

tcharding · 2024-10-13T22:03:19Z

Bother, I fixed that the other day and put the fix in the wrong commit. Added import of ToString to the first commit.

github-actions · 2024-10-13T22:04:08Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra · 2024-10-13T22:38:41Z

Will also one-ack merge this since it has no comments from other maintainers and has been open for two weeks (and Kix appears to be offline anyway).

apoelstra

ACK d649c06 successfully ran local tests

github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-primitives labels Oct 1, 2024

github-actions bot added the API break This PR requires a version bump for the next release label Oct 1, 2024

tcharding force-pushed the 10-01-mv-script-to-primitives branch from 9d05c21 to 51f6bb0 Compare October 1, 2024 03:43

tcharding mentioned this pull request Oct 1, 2024

fmt: Use style_edition = 2021 #3432

Merged

tcharding force-pushed the 10-01-mv-script-to-primitives branch from 51f6bb0 to 5758abf Compare October 1, 2024 04:12

jamillambert reviewed Oct 1, 2024

View reviewed changes

apoelstra reviewed Oct 1, 2024

View reviewed changes

tcharding force-pushed the 10-01-mv-script-to-primitives branch from 5758abf to 6c115f4 Compare October 1, 2024 21:29

tcharding force-pushed the 10-01-mv-script-to-primitives branch from 6c115f4 to c2773fd Compare October 10, 2024 00:08

tcharding force-pushed the 10-01-mv-script-to-primitives branch from c2773fd to ef11581 Compare October 10, 2024 00:28

tcharding added 2 commits October 14, 2024 09:02

Inline bytes_to_asm_fmt into Script::Display impl

ec46359

This function does not need to exist anymore because it is exactly the same as what is produced by the `Display` impl.

Move script types to primitives

d649c06

Move the `Script` and `ScriptBuf` types to `primitives`, nothing else.

tcharding force-pushed the 10-01-mv-script-to-primitives branch from ef11581 to d649c06 Compare October 13, 2024 22:02

apoelstra approved these changes Oct 13, 2024

View reviewed changes

apoelstra merged commit 1b23d1c into rust-bitcoin:master Oct 14, 2024

tcharding deleted the 10-01-mv-script-to-primitives branch October 14, 2024 22:19

tcharding mentioned this pull request Oct 18, 2024

Tracking issue: Release primitives v0.101.0 #3035

Closed

8 tasks

Conversation

tcharding commented Oct 1, 2024

Uh oh!

github-actions bot commented Oct 1, 2024

Uh oh!

github-actions bot commented Oct 1, 2024

Uh oh!

github-actions bot commented Oct 1, 2024

Uh oh!

jamillambert left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

tcharding Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

tcharding Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

apoelstra Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

apoelstra Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

tcharding Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 1, 2024

Uh oh!

tcharding commented Oct 1, 2024

Uh oh!

github-actions bot commented Oct 1, 2024

Uh oh!

tcharding commented Oct 1, 2024

Uh oh!

github-actions bot commented Oct 10, 2024

Uh oh!

github-actions bot commented Oct 10, 2024

Uh oh!

tcharding commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Oct 13, 2024

Uh oh!

tcharding commented Oct 13, 2024

Uh oh!

github-actions bot commented Oct 13, 2024

Uh oh!

apoelstra commented Oct 13, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Oct 13, 2024 •

edited

Loading