Make Address::get_payload_bytes public #978

fmeringdal · 2022-04-29T10:02:10Z

Hi, thanks for the amazing work on this crate.

I am trying to upgrade from v0.27 to v0.28, but unable to do so because the Address::get_payload_bytes was made private. My use-case is that I have a script hash address and an Address and need to compare the two, and in order to do so I need access to the payload bytes of Address.
I hope you will consider making this function public again 🙏

sanket1729

utACK 525ea00

tcharding · 2022-05-01T22:46:21Z

Thanks for the contribution!

Since Payload is a public field on Address perhaps we can have an as_bytes method on Payload and the removal of payload_as_bytes method all together?

I am trying to upgrade from v0.27 to v0.28, but unable to do so because the Address::get_payload_bytes was made private.

To increase my understanding I went looking for the change you mention but get_payload_bytes did not exist in 0.27?

fmeringdal · 2022-05-02T10:37:00Z

Since Payload is a public field on Address perhaps we can have an as_bytes method on Payload and the removal of payload_as_bytes method all together?

Agreed, that is cleaner. Updated.

To increase my understanding I went looking for the change you mention but get_payload_bytes did not exist in 0.27?

My bad, I actually relied on a fork of this repo which had this function and I didn't notice that the function was only part of the fork and did not exist in this repo. So now that v0.28 is released it would be great to get rid of the fork.

Kixunil · 2022-05-02T11:42:37Z

I'm not sure i this is a good idea. It seems to leak the internal representation of Address. It might make future extensions harder (or break compatibility).

Kixunil · 2022-05-02T11:44:34Z

src/util/address.rs

@@ -468,6 +468,15 @@ impl Payload {
            program: output_key.as_inner().serialize().to_vec(),
        }
    }
+
+    /// Returns a byte slice of the payload


The documentation should be more detailed. I don't think a newbie would be able to understand what it does without reading the code.

This looks to be pulled verbatim from existing code that was added in #839

Kixunil · 2022-05-02T11:46:23Z

src/util/address.rs

+        match self {
+            Payload::ScriptHash(hash) => hash,
+            Payload::PubkeyHash(hash) => hash,
+            Payload::WitnessProgram { program, .. } => program,


Is this even correct? Witness version is normally part of the script and it changes the semantics of the program. Omitting it could lead to serious problems but we can't return it in a slice.

This make sense to me as a convenience function. The version is typically handled separately being a u5. The witness program itself without the version is analogous to both the ScriptHash and PubkeyHash that are also returned here. In the case of verifying that a key was used to derive an address, this makes a lot of sense to me.

I'm also a bit uncomfortable with this, but I can see both sides of the argument.

Perhaps we should add more thorough docs to this method?

sanket1729 · 2022-05-02T22:36:37Z

To increase my understanding I went looking for the change you mention but get_payload_bytes did not exist in 0.27?

Sorry for my pre-mature ACK. Initially, this seemed like a one-word change without much harm.

sanket1729 · 2022-05-02T22:44:09Z

My use-case is that I have a script hash address and an Address and need to compare the two, and in order to do so I need access to the payload bytes of Address

Sorry for not reading the issue clearly. Payload is already exposed directly in Address as it is a public member.

if let Payload::ScriptHash(addr_hash) == addr {
     if expected_script_hash == addr_hash {

    }
}

This should satisfy your use-case?

Shatnerz · 2022-05-03T08:31:45Z

Looks like I was the original author of payload_as_bytes() in this PR: #839

There is some discussion in that PR around whether the methods should be public or private.

I'm not an official maintainer, but I approve of the changes as they stand in 7ca30b6, definitely an improvement. I'm not sure that I understand the immediate concerns. I could definitely see how this is useful though.

apoelstra

ACK 7ca30b6

I think this is fine to expose.

tcharding · 2022-05-10T22:42:21Z

@apoelstra looks like you acked but forgot to approve. I'm not acking/nacking because I don't feel qualified to judge. Can we get an ack/nack from @sanket1729 and/or @Kixunil please?

apoelstra

ACK 7ca30b6

Sorry, clicked the wrong button last time. I should really figure out how to do this from the commandline..

tcharding · 2022-05-16T01:02:15Z

All the open ended comments are really improvements to the PR. The PR is still, IMO, in its current form an improvement to the code. Perhaps in the name of making the contributor experience better we can merge this one as is. We can improve the docs and stuff later on.

tcharding

ACK 7ca30b6

tcharding · 2022-05-16T01:04:13Z

I should really figure out how to do this from the commandline.

I figured it out for you, been meaning to do this myself too.

gh pr review -a -b "ACK $(git log -1 --pretty=oneline --reverse | cut -d ' ' -f 1)"

(While on the branch and commit that you want to ack.)

tcharding · 2022-05-16T01:14:06Z

And since it took me a few minutes to work it out, if you use zsh

function ack-current-commit() {
    read "ack?Really ACK this commit? "
    if [[ "$ack" =~ ^[Yy]$ ]]
    then
        gh pr review -a -b "ACK $(git log -1 --pretty=oneline --reverse | cut -d ' ' -f 1)"
    fi
}

sanket1729

ACK 7ca30b6. Sorry for the delay and congratz on your first time contribution

apoelstra · 2022-05-20T13:24:54Z

@tcharding thanks! FYI you can use git rev-parse pr/XXX/head rather than mucking around with git log :)

tcharding · 2022-05-23T22:34:59Z

Oh yes, thanks. This is much better, TIL: gh pr review -a -b "ACK $(git rev-parse @)"

…public 7ca30b6 Move Address::payload_as_bytes to Payload::as_bytes (Fredrik Meringdal) 525ea00 Make Address::get_payload_bytes public (Fredrik Meringdal) Pull request description: Hi, thanks for the amazing work on this crate. I am trying to upgrade from v0.27 to v0.28, but unable to do so because the `Address::get_payload_bytes` was made private. My use-case is that I have a script hash address and an `Address` and need to compare the two, and in order to do so I need access to the payload bytes of `Address`. I hope you will consider making this function public again 🙏 ACKs for top commit: apoelstra: ACK 7ca30b6 tcharding: ACK 7ca30b6 sanket1729: ACK 7ca30b6. Sorry for the delay and congratz on your first time contribution Tree-SHA512: 02af4565853d93506751ed7cb004f52cb5d8c7936067e06b3e237b448ccdf5716470448eeccbe211958e095b66bb37c7027800c0470c6988dc18d8bd5b48f459

Make Address::get_payload_bytes public

525ea00

sanket1729 previously approved these changes Apr 30, 2022

View reviewed changes

Move Address::payload_as_bytes to Payload::as_bytes

7ca30b6

fmeringdal dismissed sanket1729’s stale review via 7ca30b6 May 2, 2022 10:32

Kixunil reviewed May 2, 2022

View reviewed changes

apoelstra reviewed May 3, 2022

View reviewed changes

apoelstra approved these changes May 13, 2022

View reviewed changes

tcharding approved these changes May 16, 2022

View reviewed changes

sanket1729 approved these changes May 20, 2022

View reviewed changes

sanket1729 merged commit 48466bd into rust-bitcoin:master May 20, 2022

Kixunil added the minor API Change This PR should get a minor version bump label Aug 1, 2022

Kixunil added this to the 0.29.0 milestone Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Address::get_payload_bytes public #978

Make Address::get_payload_bytes public #978

fmeringdal commented Apr 29, 2022

sanket1729 left a comment

tcharding commented May 1, 2022

fmeringdal commented May 2, 2022

Kixunil commented May 2, 2022

Kixunil May 2, 2022

Shatnerz May 3, 2022

Kixunil May 2, 2022

Shatnerz May 3, 2022

apoelstra May 3, 2022

tcharding May 3, 2022

sanket1729 commented May 2, 2022

sanket1729 commented May 2, 2022

Shatnerz commented May 3, 2022

apoelstra left a comment

tcharding commented May 10, 2022

apoelstra left a comment

tcharding commented May 16, 2022

tcharding left a comment

tcharding commented May 16, 2022 •

edited

Loading

tcharding commented May 16, 2022

sanket1729 left a comment

apoelstra commented May 20, 2022

tcharding commented May 23, 2022

Make Address::get_payload_bytes public #978

Make Address::get_payload_bytes public #978

Conversation

fmeringdal commented Apr 29, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding commented May 1, 2022

fmeringdal commented May 2, 2022

Kixunil commented May 2, 2022

Kixunil May 2, 2022

Choose a reason for hiding this comment

Shatnerz May 3, 2022

Choose a reason for hiding this comment

Kixunil May 2, 2022

Choose a reason for hiding this comment

Shatnerz May 3, 2022

Choose a reason for hiding this comment

apoelstra May 3, 2022

Choose a reason for hiding this comment

tcharding May 3, 2022

Choose a reason for hiding this comment

sanket1729 commented May 2, 2022

sanket1729 commented May 2, 2022

Shatnerz commented May 3, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented May 10, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented May 16, 2022

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented May 16, 2022 • edited Loading

tcharding commented May 16, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra commented May 20, 2022

tcharding commented May 23, 2022

tcharding commented May 16, 2022 •

edited

Loading