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

Handle v2 error generic #473

Closed
wants to merge 2 commits into from

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Jan 9, 2025

Make the Receiver state machine generic so that it the error handling function can always provide context to an error

Extract JSON OHTTP request to be returned to the directory

This is a Draft because I'm not sure which design is going to make the most sense yet. Sharing my exploration of way to close #468

@DanGould DanGould force-pushed the handle-v2-error-generic branch from 0be8384 to 486ccfe Compare January 10, 2025 05:07
@coveralls
Copy link
Collaborator

coveralls commented Jan 10, 2025

Pull Request Test Coverage Report for Build 12739793767

Details

  • 84 of 174 (48.28%) changed or added relevant lines in 5 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.6%) to 60.261%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/receive/v2/error.rs 0 3 0.0%
payjoin-cli/src/db/v2.rs 0 4 0.0%
payjoin/src/receive/error.rs 0 6 0.0%
payjoin-cli/src/app/v2.rs 0 29 0.0%
payjoin/src/receive/v2/mod.rs 84 132 63.64%
Files with Coverage Reduction New Missed Lines %
payjoin-cli/src/app/v2.rs 1 0.0%
payjoin-cli/src/db/v2.rs 1 0.0%
payjoin/src/receive/v2/mod.rs 3 80.48%
Totals Coverage Status
Change from base Build 12677131350: -0.6%
Covered Lines: 2910
Relevant Lines: 4829

💛 - Coveralls

@DanGould DanGould force-pushed the handle-v2-error-generic branch from 486ccfe to 84572ba Compare January 13, 2025 02:55
This allows us to share common functions with every state in preparation
to handle errors with data from the SessionContext.
Recoverable errors can be shared with the Sender rather than that
Receiver going offline.
@DanGould
Copy link
Contributor Author

This works.

However,

after updating, I realize that in the implementation only the Receiver<InitialState> needed to use the pub fn extract_err_req, process_err_res if the error is propagated up to the caller. So I think I'm going to rework this not to be generic and to simply handle the error at that higher level.

@0xBEEFCAF3
Copy link
Collaborator

This works.

However,

after updating, I realize that in the implementation only the Receiver<InitialState> needed to use the pub fn extract_err_req, process_err_res if the error is propagated up to the caller. So I think I'm going to rework this not to be generic and to simply handle the error at that higher level.

I'm not sure if you going to revert your generic work in 3534835 based on this comment. This work would potentially reduce the code footprint in multiparty things. Especially if we make sender context an associated type.

@DanGould
Copy link
Contributor Author

Yes @0xBEEFCAF3, I made a more minimal implementation of this idea in #474. Which side of the fence would "potentially reduce the code footprint in multiplarty things," and how?

Could you explain what you mean about making sender context an associated type? Or do you mean SessionContext?

@DanGould
Copy link
Contributor Author

The generic part of this doesn't even come into play with the implementation, so I'm going to close this for now. If it turns out that making a Receiver accept an associated SessionContext from either v2 or v3 I'm cool to reopen this

@DanGould DanGould closed this Jan 13, 2025
@0xBEEFCAF3
Copy link
Collaborator

Yes @0xBEEFCAF3, I made a more minimal implementation of this idea in #474. Which side of the fence would "potentially reduce the code footprint in multiplarty things," and how?

Could you explain what you mean about making sender context an associated type? Or do you mean SessionContext?

As I was working on the multiparty state machine I noticed that its essentially the same as the v2 reciever state machine (so far... I havent worked on any real validation so it may be the case that there needs to be additional / fewer states). The only real difference is the SessionContext type on the v2 reciever state structs (which needs to be a list of sender session contexts)
If we used traits and associated types that allows me to re-use the v2 recievers and just redefine SessionContext. I would say the demand for this level of change pretty low. I can still get what I want done and doing this refactor would just be a pre-optimization

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.

V2 Original Message A error json not returned to Sender from Receiver
3 participants