Skip to content

Impl WellKnownError::additional_data - #182

Closed
jbesraa wants to merge 1 commit into
payjoin:masterfrom
jbesraa:feat/wellknownerror-additional-data-getter
Closed

Impl WellKnownError::additional_data#182
jbesraa wants to merge 1 commit into
payjoin:masterfrom
jbesraa:feat/wellknownerror-additional-data-getter

Conversation

@jbesraa

@jbesraa jbesraa commented Jan 29, 2024

Copy link
Copy Markdown
Contributor

resolves #181

  Retrives additional data that could be returned
  by the receiver to help the payjoin sender amend
  their requer.
  Currently, only in `WellKnownError:VersionUnsupported`
  We do return this kind of additional data including
  the supported version the payjoin receiver allow.
@jbesraa
jbesraa requested a review from DanGould January 29, 2024 11:30
@DanGould

Copy link
Copy Markdown
Member

Seems like we want to match a newly exposed WellKnownError instead since those errors are by definition well known and part of the api to handle

#151 (comment)

@jbesraa

jbesraa commented Jan 30, 2024

Copy link
Copy Markdown
Contributor Author

As we already expose the parameters of the WellKnownError through error_code and message, keeping the same structure instead of making WellKnownError public felt a better choice for me. I think also exposing less variables, while exposing the required functionality makes more sense.

Moreover, as WellKnown error_codes(version-unsupported, original-psbt-,,,) are predefined in BiP0078, users can
match error.error_code() { "version-unsupported" => { } ... }

@DanGould

DanGould commented Jan 30, 2024

Copy link
Copy Markdown
Member

Matching on a magic string is an opaque api, so we can't do that, we'd have to expose a const, but I don't think we should. WellKnownError is already part of the public ResponseError api, so perhaps it should be public outside of this issue.

I'd definitely not expect Vec as additional_data, since we're parsing arbitrary json blobs that can give us arbitrary data.

What if we rethink all of this matching to occur within the ResponseError api and expose a ResponseError::wants_old_version(&self, v) -> bool that returns if our old version is supported

or ResponseError::wants_old_version(&self, supported: Vec<u64>) -> Vec<u64> that returns the conjunction of the error set and our supplied set then we can switch on that while being concise in the purpose of the method call in docs. I recommend either of these approaches.

PS renaming the title of this PR to describe the problem it solves & why vs what could help us stay flexible in solutions too.

@jbesraa jbesraa closed this Feb 1, 2024
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.

Expose additional data from WellKnownError

2 participants