Enumerate errors for peek_with_timeout

[shinghim]

I returned the same values for the new Error case that were previously returned for a None in the peek_default and peek_v1 usages to maintain the same behavior. Maybe it'd be useful to update them to say something about a timeout?

Fixes #396

[coveralls]

Pull Request Test Coverage Report for Build 12614809624

Details

• 13 of 26 (50.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.08%) to 61.624%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-directory/src/db.rs	7	8	87.5%
payjoin-directory/src/lib.rs	6	8	75.0%
payjoin-directory/src/error.rs	0	10	0.0%

Totals
Change from base Build 12607171939:	-0.08%
Covered Lines:	2945
Relevant Lines:	4779

---

💛 - Coveralls

[DanGould]

concept ACK 7f27c9d, though it seems like a complete this would really entail defining an Error enum containing both RedisResult's RedisError and Elapsed rather than the Ok value being a RedisResult.

Perhaps add one more commit that does that.

A docstring describing the behavior of peek_v1 (or any function) is absolutely appropriate and encouraged.

[shinghim]

Ah makes sense, ill update this hopefully tomorrow

[DanGould]

Nice, this is what I was looking for.

I think it would be more appropriate for the DbPoolError to be defined in db.rs, (potentially named db::Error) since it's not an error on the whole lib, but rather the db module specifically. Additionally, I would expect a payjoin-directory/src/error.rs to host HandlerError defined as lib.rs too, and since there's only one error type it may not be worth the overhead of new files and imports for the time being.

I also see an opportunity to reduce duplicate code in lib.rs since the pattern is repeated:

fn handle_peek(
    result: Result<Vec<u8>, DbPoolError>,
    timeout_response: Response<BoxBody<Bytes, hyper::Error>>,
) -> Result<Response<BoxBody<Bytes, hyper::Error>>, HandlerError> {
    match result {
        Ok(buffered_req) => Ok(Response::new(full(buffered_req))),
        Err(e) => match e {
            DbPoolError::Redis(re) => Err(HandlerError::BadRequest(re.into())),
            DbPoolError::Timeout(_) => Ok(timeout_response),
        },
    }
}

May you follow up with the module cleanup (putting the new error in db.rs)? The deduplication is optional, since duplicate code is already contained in this repo and keeping it around would not be a regression. But i'd like to see it :)

Thanks for the persistent contributions 🫡

[DanGould]

IF we want to return the Redis error to the client (big IF, @nothingmuch what do you think) I think we can return Redis(e) => Err(HandlerError::BadRequest(e.into()) rather than the top level error, since that's the only returned variant. Then Err(_) => match e is appropriate at the higher level.

[shinghim]

Is the concern here that Redis will be replaced in the near future (#419), or just generally that the client shouldn't have to worry about the underlying implementation?

[DanGould]

exposing internals of the server

1. isn't generally actionable by most callers
2. should just be error logged instead and
3. exposes details about the server state that potentially could be used for an attack. though I'm not sure I can think of one off the top of my head, my idea of best practice is to reveal only that necessary and actionable serverstate for the client to operate and keep everything else private.

[DanGould]

I think this needs to be pub(crate) since we don't plan to expose it in the public API.

[shinghim]

Feedback makes sense, I'll move the error and add the dedupe. I'll wait until the Redis error propagation discussion is resolved before pushing all of the updates here at once