Make Script::fmt_asm a static method and add Script::str_asm #569

stevenroose · 2021-02-10T13:27:49Z

This makes it convenient to print/construct the script assembly on byte slices withoout having to clone them to copy them to create a Script struct.

sgeisler · 2021-02-10T13:48:29Z

src/blockdata/script.rs

-    /// Get the assembly decoding of the script.
-    pub fn asm(&self) -> String {
+    /// Create an assembly decoding of the script in the given byte slice.
+    pub fn str_asm(script: &[u8]) -> String {


Would asm_from_bytes be a better name? Both asm and str_asm return Strings.

Hmm yeah naming. I figured not too important is't not really gonna be a high-usage method. asm is a member method, that would be used more. I made str_asm kinda cfr fmt_asm, but asm_from_bytes makes sense.

I think the good naming for the associated conversion function should include both original and return data type, like bytes_to_string (str is reserved for &str return type). Thus, it will be used as Script::bytes_to_string, which is very readable and explicit.

I disagree on the return type being in the name, string is as non-descriptive as it gets (could be hex for all we know), this should remain the chosen encoding, i.e. asm. I'd be ok with bytes_to_asm, but don't really care as long as it's descriptive.

Agree on bytes_to_asm

@stevenroose Would you be ok with giving this a more descriptive name? Rarely used or not, it is part of the public API we will be stuck with for a while.

sgeisler · 2021-02-10T13:51:27Z

src/blockdata/script.rs

@@ -433,42 +439,42 @@ impl Script {
    }

    /// Write the assembly decoding of the script to the formatter.
-    pub fn fmt_asm(&self, f: &mut dyn fmt::Write) -> fmt::Result {
+    pub fn fmt_asm(script: &[u8], f: &mut dyn fmt::Write) -> fmt::Result {


This PR could be backwards compatible if you renamed that fn to fmt_asm_bytes (or similar) and implemented fmt_asm based on that, like you did with asm.

Is that worth the effort, you think? The fmt asm method is really already a helper that I don't think has any real users. I can wait until we do a breaking release for this to be included.

As it is this is a breaking change as far as I can tell since fmt_asm is public. I think this little change could make this PR non-breaking, allowing us to change the milestone to 0.26.1. Afaik we wanted to get away from frequent API breaks, so there will probably be some 0.26.x releases to get new APIs out before any major refactoring in 0.27.0.

dr-orlovsky

Agree with @sgeisler in his requested changes

src/blockdata/script.rs

sanket1729

utACK 2cb8185 . I have no strong opinions on naming, as 1) it is a debug/test method and 2) can be easily seen by looking up docs.

It will be a breaking change for rust-miniscript because we use this in our tests, but it should not be hard to fix.

This makes it convenient to print/construct the script assembly on byte slices withoout having to clone them to copy them to create a Script struct.

stevenroose · 2021-03-13T22:16:58Z

I changed the names. And rebased.

apoelstra

ack 851a3a1

dr-orlovsky

Ok, agree with this version

sgeisler

tACK 851a3a1

Test log:

Mar 27 11:55:30.957  INFO testinator: Installing rust toolchain 'nightly'
Mar 27 11:55:49.972  INFO testinator: Installing rust toolchain 'stable'
Mar 27 11:56:54.896  INFO testinator: Installing rust toolchain '1.29.0'
Mar 27 11:56:55.916  INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Mar 27 11:56:55.916  INFO testinator: Preparing environment for rust stable tests (8 configurations)
Mar 27 11:56:55.916  INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Mar 27 11:59:19.513  INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.Cf3vnJcUsHEA/rust-bitcoin
Mar 27 11:59:19.513 DEBUG testinator: Generating lock file with rust=1.29.0
Mar 27 11:59:19.519  INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.o31ZRDPsAqge/rust-bitcoin
Mar 27 11:59:19.522  INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.e6g5fj0QAChO/rust-bitcoin
Mar 27 12:00:19.701 DEBUG testinator: Pinning cc to 1.0.41
Mar 27 12:00:20.015 DEBUG testinator: Pinning serde to 1.0.98
Mar 27 12:00:20.167 DEBUG testinator: Pinning serde_derive to 1.0.98
Mar 27 12:00:20.319 DEBUG testinator: Pinning byteorder to 1.3.4
Mar 27 12:01:18.231  INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Mar 27 12:01:25.878  INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Mar 27 12:01:54.922  INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Mar 27 12:02:23.395  INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Mar 27 12:02:45.540  INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Mar 27 12:02:52.858  INFO testinator: Test rust=nightly, features=[base64] succeeded!
Mar 27 12:03:17.262  INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Mar 27 12:03:17.285  INFO testinator: Test rust=stable, features=[base64] succeeded!
Mar 27 12:03:58.190  INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Mar 27 12:03:58.738  INFO testinator: Test rust=nightly, features=[rand] succeeded!
Mar 27 12:03:58.870  INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Mar 27 12:04:31.496  INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Mar 27 12:04:34.408  INFO testinator: Test rust=stable, features=[rand] succeeded!
Mar 27 12:04:50.245  INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Mar 27 12:05:01.139  INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Mar 27 12:05:22.010  INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Mar 27 12:05:37.926  INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!                                                                                                  
Mar 27 12:05:43.496  INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Mar 27 12:05:59.844  INFO testinator: Test rust=nightly, features=[] succeeded!
Mar 27 12:06:08.202  INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                            
Mar 27 12:06:30.850  INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Mar 27 12:06:33.799  INFO testinator: Test rust=stable, features=[] succeeded!
Mar 27 12:07:20.275  INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Mar 27 12:08:10.412  INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                            
Mar 27 12:08:52.316  INFO testinator: Test rust=1.29.0, features=[] succeeded!
Mar 27 12:08:56.460  INFO testinator: Fuzzing deserialize_script
Mar 27 12:10:10.226  INFO testinator: Successfully fuzzed deserialize_script
Mar 27 12:10:10.226  INFO testinator: Fuzzing uint128_fuzz
Mar 27 12:11:12.186  INFO testinator: Successfully fuzzed uint128_fuzz
Mar 27 12:11:12.186  INFO testinator: Fuzzing deserialize_amount
Mar 27 12:12:13.210  INFO testinator: Successfully fuzzed deserialize_amount
Mar 27 12:12:13.211  INFO testinator: Fuzzing deserialize_transaction
Mar 27 12:13:15.198  INFO testinator: Successfully fuzzed deserialize_transaction
Mar 27 12:13:15.198  INFO testinator: Fuzzing deser_net_msg
Mar 27 12:14:18.252  INFO testinator: Successfully fuzzed deser_net_msg
Mar 27 12:14:18.253  INFO testinator: Fuzzing deserialize_address
Mar 27 12:15:19.242  INFO testinator: Successfully fuzzed deserialize_address
Mar 27 12:15:19.242  INFO testinator: Fuzzing deserialize_block
Mar 27 12:16:21.104  INFO testinator: Successfully fuzzed deserialize_block
Mar 27 12:16:21.104  INFO testinator: Fuzzing outpoint_string
Mar 27 12:17:22.236  INFO testinator: Successfully fuzzed outpoint_string
Mar 27 12:17:22.236  INFO testinator: Fuzzing deserialize_psbt
Mar 27 12:18:25.179  INFO testinator: Successfully fuzzed deserialize_psbt

sgeisler · 2021-03-27T13:56:52Z

I also think this version isn't API breaking anymore, am I right? @stevenroose

Make Script::fmt_asm a static method and add Script::str_asm

stevenroose added the API break This PR requires a version bump for the next release label Feb 10, 2021

stevenroose changed the title ~~Static asm~~ Make Script::fmt_asm a static method and add Script::str_asm Feb 10, 2021

sgeisler reviewed Feb 10, 2021

View reviewed changes

dr-orlovsky suggested changes Feb 11, 2021

View reviewed changes

src/blockdata/script.rs Show resolved Hide resolved

src/blockdata/script.rs Outdated Show resolved Hide resolved

sanket1729 previously approved these changes Feb 26, 2021

View reviewed changes

stevenroose added 2 commits March 13, 2021 22:13

Implement AsRef<[u8]> for Script

96fa4b7

Make Script::fmt_asm a static method and add Script::str_asm

851a3a1

This makes it convenient to print/construct the script assembly on byte slices withoout having to clone them to copy them to create a Script struct.

stevenroose dismissed sanket1729’s stale review via 851a3a1 March 13, 2021 22:16

stevenroose force-pushed the static-asm branch from 2cb8185 to 851a3a1 Compare March 13, 2021 22:16

apoelstra approved these changes Mar 23, 2021

View reviewed changes

dr-orlovsky approved these changes Mar 26, 2021

View reviewed changes

sgeisler approved these changes Mar 27, 2021

View reviewed changes

sgeisler removed the API break This PR requires a version bump for the next release label Mar 31, 2021

sgeisler added this to the 0.26.1 milestone Mar 31, 2021

sgeisler merged commit 1326f7d into rust-bitcoin:master Mar 31, 2021

darosior pushed a commit to darosior/rust-bitcoin that referenced this pull request Apr 6, 2021

Merge pull request rust-bitcoin#569 from stevenroose/static-asm

ac82fd7

Make Script::fmt_asm a static method and add Script::str_asm

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Script::fmt_asm a static method and add Script::str_asm #569

Make Script::fmt_asm a static method and add Script::str_asm #569

stevenroose commented Feb 10, 2021 •

edited

sgeisler Feb 10, 2021

stevenroose Feb 10, 2021

dr-orlovsky Feb 11, 2021 •

edited

sgeisler Feb 12, 2021

dr-orlovsky Feb 12, 2021

sgeisler Feb 26, 2021

sgeisler Feb 10, 2021

stevenroose Feb 25, 2021

sgeisler Feb 26, 2021 •

edited

stevenroose Mar 13, 2021

dr-orlovsky left a comment

sanket1729 left a comment

stevenroose commented Mar 13, 2021

apoelstra left a comment

dr-orlovsky left a comment

sgeisler left a comment •

edited

sgeisler commented Mar 27, 2021

Make Script::fmt_asm a static method and add Script::str_asm #569

Make Script::fmt_asm a static method and add Script::str_asm #569

Conversation

stevenroose commented Feb 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky Feb 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler Feb 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

stevenroose commented Mar 13, 2021

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

sgeisler left a comment • edited

Choose a reason for hiding this comment

sgeisler commented Mar 27, 2021

stevenroose commented Feb 10, 2021 •

edited

dr-orlovsky Feb 11, 2021 •

edited

sgeisler Feb 26, 2021 •

edited

sgeisler left a comment •

edited