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

Audit function signatures #405

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

Conversation

DanGould
Copy link
Contributor

No description provided.

Co-authored-by: spacebear21 <[email protected]>
Co-authored-by: nothingmuch <[email protected]>
@DanGould DanGould marked this pull request as draft November 23, 2024 17:21
@DanGould
Copy link
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
#[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
Collaborator

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 =>

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
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

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
Contributor Author

Choose a reason for hiding this comment

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

yes

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.

3 participants