-
Notifications
You must be signed in to change notification settings - Fork 332
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
WIP Connection queries for Relayer #136
Changes from 10 commits
7e6e76a
030c4d0
ae792c0
d332fd6
949ad61
107cccb
3aae417
9c2f528
7423376
4db544c
a8ed4d5
f8c0721
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,15 @@ impl State { | |
Self::Open => "OPEN", | ||
} | ||
} | ||
|
||
pub fn from_i32(nr: i32) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ancazamfir was not sure this is accurate, please double-check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good. |
||
match nr { | ||
1 => Self::Init, | ||
2 => Self::TryOpen, | ||
3 => Self::Open, | ||
_ => Self::Uninitialized, | ||
} | ||
} | ||
} | ||
|
||
impl std::str::FromStr for State { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ impl CommitmentPath { | |
where | ||
P: Path, | ||
{ | ||
todo!() | ||
CommitmentPath {} | ||
} | ||
} | ||
|
||
|
@@ -32,6 +32,10 @@ impl CommitmentProof { | |
*/ | ||
|
||
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | ||
pub struct CommitmentPrefix; | ||
pub struct CommitmentPrefix(Vec<u8>); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if we should have a formatter for this. The output of the query shows: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented a fmt::Debug trait to properly format the CommitmentPrefix output in this commit |
||
|
||
// TODO: impl CommitPrefix | ||
impl CommitmentPrefix { | ||
pub fn new(content: Vec<u8>) -> Self { | ||
Self { 0: content } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get rid of the
unwrap()
s? If the user issues a query command and gets back a few pages of traces it does not help. I had this comment earlier but I guess it got lost. Try to query with non-exsiting connection id.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to make this into a From trait, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I guess this should be verifying all the fields, not just 2? And should we really have an error per field or maybe start with a more general InvalidConnectionEnd error of some kind and include what's missing as a string? Not sure ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either option could work, the question is if we want to keep validation inside the
from
or outside. The current code uses theFromStr
trait, with the signaturefn from_str(s: &str) -> Result<Self, Self::Err>
so we can do validation inside there and return error if validation fails. WithFrom
, the sig isfn from(T) -> Self
so we'd have to do validation after callingfrom
. Is it generally speaking, or specifically in this case, better to do validation outside offrom
?LE: Maybe this helps?
Agree this deserves a discussion to find a more principled approach.