Conversation
arminsabouri
force-pushed
the
recv-session-event
branch
from
9dcca47 to
6631cf7
Compare
Pull Request Test Coverage Report for Build 15589279796Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
Two comments from Dan's review on the other PR should be addressed here imo:
I also think it would make sense to address this comment here:
| /// Session is invalid. This is a irrecoverable error. Fallback tx should be broadcasted. | ||
| /// TODO this should be any error type that is impl std::error and works well with serde, or as a fallback can be formatted as a string | ||
| /// Reason being in some cases we still want to preserve the error b/c we can action on it. For now this is a terminal state and there is nothing to replay and is saved to be displayed. | ||
| /// b/c its a terminal state and there is nothing to replay. So serialization will be lossy and that is fine. |
There was a problem hiding this comment.
I don't really understand the TODO here, maybe this needs to be reworded? Is it a TODO because it needs the rest of the SEL changes to be implemented first?
There was a problem hiding this comment.
No.
The TODO is saying: instead of a string repr of the error it should be any error type that plays nicely with serde. In the future we may want to take action on why a session closed. However its a TODO because we dont impl serde::serialize/deserialize on our error types.
Comment addressed in #750 @spacebear21 I'm not sure how to address this one here. The session history struct doesnt exist yet.
@spacebear21 can do! If its ok with you I will make this its own atomic commit. |
My bad, I meant to link to the second reply:
I think moving these to |
arminsabouri
force-pushed
the
recv-session-event
branch
from
6631cf7 to
79213ad
Compare
This commit adds deserialization support for the version enum so it can be stored in the session event log.
This commit adds session events generated by processing the receiver state machine. These events wrap v1 receiver types to avoid duplicating session context learned during the initial typestate transition. To enable serialization and deserialization of session events, we implement serde::Serialize and serde::Deserialize on the underlying v1 types. As a result, we must enable the bitcoin/serde feature under the v1 flag because `OptionalParameters` includes a bitcoin::FeeRate field. To enable comparisons between SessionEvents during tests, all event types must also implement `PartialEq` and `Eq`.
This commit adds test coverage for the full serialization/deserialization round trip for each receiver session event.
arminsabouri
force-pushed
the
recv-session-event
branch
from
79213ad to
c7795cd
Compare
3 participants
This PR adds session events generated by processing the receiver
state machine. These events wrap v1 receiver types to avoid duplicating
session context learned during the initial typestate transition. To
enable serialization and deserialization of session events, we implement
serde::Serialize and serde::Deserialize on the underlying v1 types. As a
result, we must enable the bitcoin/serde feature under the v1 flag
because
OptionalParametersincludes a bitcoin::FeeRate field.To enable comparisons between SessionEvents during tests,
all event types must also implement
PartialEqandEq.Tests cover roundtrip ser/deserialization of each session event.
These commits were cherry-picked off #750 and cleaned up.