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

invoice: Getter for fallbacks as Address'es #882

Closed
sgeisler opened this issue Apr 13, 2021 · 6 comments · Fixed by #2023
Closed

invoice: Getter for fallbacks as Address'es #882

sgeisler opened this issue Apr 13, 2021 · 6 comments · Fixed by #2023
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@sgeisler
Copy link
Collaborator

The fallback() method returns some custom address payload implementation. A regular fallback_addresses or so that is just a Vec<Address> would be great.

Or an Into<Address> for Fallback.

@futurepaul
Copy link
Contributor

is there any good way right now to convert a Fallback into an Address? just ran into this and not sure the best strat.

@TheBlueMatt TheBlueMatt added this to the 0.0.115 milestone Feb 7, 2023
@TheBlueMatt
Copy link
Collaborator

Sadly only pretty manually, we should fix this!

@futurepaul
Copy link
Contributor

I'll make an Into<Address> based on this logic:

    match fallback {
        Fallback::SegWitProgram { version, program } => {
            let version = WitnessVersion::from_u5(version).unwrap();
            let script = Script::new_witness_program(version, program.as_slice());
            Address::from_script(&script, Network::Bitcoin)
        }
        Fallback::PubKeyHash(pkh) => {
            let pkh = PubkeyHash::from_slice(&pkh).unwrap();
            let script = Script::new_p2pkh(&pkh);
            Address::from_script(&script, Network::Bitcoin)
        }
        Fallback::ScriptHash(sh) => {
            let sh = ScriptHash::from_slice(&sh).unwrap();
            let script = Script::new_p2sh(&sh);
            Address::from_script(&script, Network::Bitcoin)
        }
    }

@jkczyz jkczyz assigned jkczyz and unassigned stevenroose Feb 8, 2023
@jkczyz
Copy link
Contributor

jkczyz commented Feb 8, 2023

Currently, the builder has a method that takes a Fallback, which are then exposed via an accessor returning Vec<&Fallback>. So an Into<Address> would require a clone since self is taken by value.

We have a few options that I can imagine:

  • use the Into approach and live with users needing to clone
  • hide Fallback by having separate builder methods for each variant and have the accessor return Vec<Address> like BOLT 12 invoices do
  • have an inconsistent interface where the builder remains as it is and the accessor returns Vec<Address>
  • add an additional accessor that returns Vec<Address> similar to private_route / route_hints, which would essentially hide the clone from the user

I'm sorta leaning towards the last option to avoid too much change in the interface, but I'm open to other options.

@futurepaul
Copy link
Contributor

sorry I saw this comment after writing this code so thought I'd throw it up here and if it's helpful can iterate, otherwise feel free to close

@jkczyz
Copy link
Contributor

jkczyz commented Feb 8, 2023

sorry I saw this comment after writing this code so thought I'd throw it up here and if it's helpful can iterate, otherwise feel free to close

Nah, we'll need that or something like that somewhere, regardless. Just need to figure out which interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants