Skip to content
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

feat: add to_address_data to address type #671

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reez
Copy link
Collaborator

@reez reez commented Feb 12, 2025

Description

Resolve #669 with to_address_data

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@reez reez linked an issue Feb 12, 2025 that may be closed by this pull request
@reez reez self-assigned this Feb 12, 2025
@reez
Copy link
Collaborator Author

reez commented Feb 12, 2025

@thunderbiscuit

@reez reez marked this pull request as ready for review February 12, 2025 16:08
@reez reez requested a review from thunderbiscuit February 12, 2025 16:08
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Looks great!

Only one small comment/idea.

As for the non-exhaustive, if you add that qualifier to the UDL does it let you do away with the _ => unimplemented branch?

@@ -26,6 +29,19 @@ use std::ops::Deref;
use std::str::FromStr;
use std::sync::{Arc, Mutex};

#[derive(Debug)]
pub enum AddressData {
P2pkh { pubkey_hash: Vec<u8> },
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that maybe what people want here is the hash in string format? Really it's not a problem because I can transform the bytes into a hex locally but I think the Hash type maybe handles some of that (big-endianness and little-endianness) when converting to strings? We also return a string rather than the bytes when giving out the Txid (which is also a Hash).

Let me know what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I agree with what you're saying but I was also trying to balance it with how Conor was using it (which was the initial reason for exposing this method) which he mentioned was this:

"Need it to to generate a valid shutdown script for closing Lightning channels"

override fun get_shutdown_scriptpubkey(): Result_ShutdownScriptNoneZ {
        val address = ldkkeysManager!!.wallet.getLastUnusedAddress().address

        return when (val payload: Payload = address.payload()) {
            is Payload.WitnessProgram -> {
                val ver = when (payload.version.name) {
                    in "V0".."V16" -> payload.version.name.substring(1).toIntOrNull() ?: 0
                    else -> 0 // Default to 0 if it doesn't match any "V0" to "V16"
                }

                val result = ShutdownScript.new_witness_program(
                    WitnessProgram(
                        payload.program.toUByteArray().toByteArray(),
                        WitnessVersion(ver.toByte())
                    )
                )
                Result_ShutdownScriptNoneZ.ok((result as Result_ShutdownScriptInvalidShutdownScriptZ.Result_ShutdownScriptInvalidShutdownScriptZ_OK).res)
            }

            else -> {
                Result_ShutdownScriptNoneZ.err()
            }
        }
    }

What are your thoughts with that added context?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the witness program I'd keep as bytes. I was more thinking of the pubkey_hash and script_hash as hex strings the way we often see/think of hashes.

Copy link
Member

Choose a reason for hiding this comment

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

But honestly I'm ok with Byte arrays! Just a thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! yeah no what you're saying makes perfect sense to me now. pubkey_hash and script_hash have been updated/changed to strings

@reez
Copy link
Collaborator Author

reez commented Feb 13, 2025

Looks great!

Only one small comment/idea.

As for the non-exhaustive, if you add that qualifier to the UDL does it let you do away with the _ => unimplemented branch?

Did you happen to get this to compile if you tried this, the only reason I'm asking is because I somehow could not get it to compile (error NonExhaustive not supported for interface definition) if I tried this even though to my understanding it actually should compile based on https://mozilla.github.io/uniffi-rs/0.28/udl/enumerations.html#remote-non-exhaustive-enums

@reez reez force-pushed the feat/address-data branch from 63840ee to 889b70a Compare February 13, 2025 21:01
@thunderbiscuit
Copy link
Member

@reez ah yes you're right. This is only possible with simple enums, not interface-based enums (which turn into Sealed Classes in Kotlin, can't remember what in Swift).

Ok this is good to go IMO!

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 889b70a.

But yes if you have maybe a test showing all 3 variants of the enum (even if it's just in Swift) that'd be great I think!

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.

Add Address.payload() method
2 participants