Skip to content

Silentpayments refactor#125

Open
Sosthene00 wants to merge 11 commits into
masterfrom
silentpayments_refactor
Open

Silentpayments refactor#125
Sosthene00 wants to merge 11 commits into
masterfrom
silentpayments_refactor

Conversation

@Sosthene00
Copy link
Copy Markdown
Collaborator

Long overdue refactoring of silentpayments crate, some changes were things I wanted to do for a long time, others are made to prepare for the psbt integration.

Most significant ones are:

  • f8a674d and 6714670 that introduces a new input type to streamline the generation of recipient pubkeys
  • ade557b that exposes a public API to "normalize" (i.e. guarantee eveness) of private keys used in inputs spending taproot outputs, which is needed for the psbt workflow. On the same occasion I tried to provide stronger guarantees and guidance for consumer of the library regarding the said normalization of keys after it proved to be a source of confusing bugs while working on psbt.

The rest is mostly incremental improvements that can be discussed case by case.

…PubkeysInput`

docs(sending): update docs for generate_recipient_pubkeys
* use bytes array instead of (String, u32)
* enforce at least 1 outpoint passed as argument, prevents empty outpoints error
* also updates calling sites in tests and examples
* new `TypedSecretKey<State>` type that keep track of changes made to a secret key and help enforce the protocol
* `NonEmptyNormalizedKeys` type prevents empty keys array errors
* taproot key normalization is now accessible as a public API, not buried inside a private function `get_a_sum_secret_key`
* unrelated, but commited for convenience: `calculate_partial_secret` takes a secp context as argument
fn try_from(value: u8) -> std::result::Result<Self, Self::Error> {
match value {
0 => Ok(Self::ZERO),
_ => Err(Error::GenericError("Unknwon silent payment version".to_string()))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
_ => Err(Error::GenericError("Unknwon silent payment version".to_string()))
_ => Err(Error::GenericError("Unknown silent payment version".to_string()))

pub(crate) fn calculate_input_hash(
outpoints_data: &[(String, u32)],
pub fn calculate_input_hash(
outpoint_head: &[u8; OUTPOINTS_LEN],
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

One refactoring change that I've been wanting to make for a long time is to use a custom OutPoint struct for our public api functions where we're expecting an outpoint struct. Maybe we can use that here too?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We could, I was using bytes array though because that's not making a lot of assumptions but I agree that an ad hoc Outpoint type that acts as a thin wrapper around that bytes array and add some build-time guarantees is a good idea

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The real issue is that we can't rely on rust-bitcoin types so letting the caller the responsibility of using serialize(&outpoint).try_into().unwrap() seemed a good idea to me, maybe just document it better. The only risk I see is caller passing a null outpoint, but is it a real risk though?

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