Skip to content

Audit function signatures#405

Closed
DanGould wants to merge 1 commit intopayjoin:masterfrom
DanGould:audit-fn-signatures
Closed

Audit function signatures#405
DanGould wants to merge 1 commit intopayjoin:masterfrom
DanGould:audit-fn-signatures

Conversation

@DanGould
Copy link
Copy Markdown
Contributor

No description provided.

Co-authored-by: spacebear21 <git@spacebear.dev>
Co-authored-by: nothingmuch <nothingmuch@woobling.org>
@DanGould DanGould marked this pull request as draft November 23, 2024 17:21
@DanGould
Copy link
Copy Markdown
Contributor Author

DanGould commented Dec 3, 2024

Another inconsistency to resolve process_res in some places and process_response in others. And those functions have inconsistent parameter names, some body, some response, some res

Comment on lines 119 to 124
origin: Url,
#[cfg(feature = "v2")] receiver_pubkey: Option<HpkePublicKey>,
#[cfg(feature = "v2")] receiver_pubkey: Option<HpkePublicKey>, // FIXME make Option<(pk, keys, exp)>
#[cfg(feature = "v2")] ohttp_keys: Option<OhttpKeys>,
#[cfg(feature = "v2")] expiry: Option<std::time::SystemTime>,
) -> Self {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See also #431

Comment on lines +140 to 143
treat_missing_as_error: bool, // FIXME never used! remove!
) -> Result<(), InternalPsbtInputError> {
match (&self.psbtin.non_witness_utxo, &self.psbtin.witness_utxo) {
(None, None) if treat_missing_as_error =>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to always treat this as an error? I saw that every time we call this we always pass true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we can just remove it from the signature and usage entirely

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ok, so the behavior if none would just be

(None, None) => {
    Err(InternalPsbtInputError::PrevTxOut(PrevTxOutError::MissingUtxoInformation))
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes

benalleng added a commit to benalleng/rust-payjoin that referenced this pull request Jan 14, 2025
Many of these function signaures were audited in payjoin#405 and implemented
here.
benalleng added a commit to benalleng/rust-payjoin that referenced this pull request Jan 14, 2025
Many of these function signaures were audited in payjoin#405 and implemented
here.
@DanGould DanGould mentioned this pull request Jan 15, 2025
@spacebear21
Copy link
Copy Markdown
Collaborator

Closing this as it's being addressed in smaller PR chunks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants