Skip to content

Conversation

@sekulicd
Copy link
Collaborator

This adds FromOutputScript func to the payment pkg which is supposed to create address from provided output script.

@tiero @altafan please review.

sekulicd added 30 commits May 20, 2020 13:41
return script
}

func FromOutputScript(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should stay in address package and should not make use of the payment one, like a "raw" address builder.
The idea is to allow the user to retrieve and address from an output script with either address or payment. The former lacks such API, while the latter already provides the necessary ones.

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, thing is that if adding it to address pkg can use payment cause of cyclic import.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was trying to say that this function should be moved to address and its implementation must change so that it doesn't make use of payment's types/functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest you don't look at the liquidjs-lib implementation, rather this function should analyze the incoming script to understand its type and encode it to an address using the proper format (base58, bech32...).
I would remove also the blinding key argument and return always an unconfidential address from an output script to keep it simple. The user can translate it to confidential with existing types and functions of the address package if needed.

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.

2 participants