-
Notifications
You must be signed in to change notification settings - Fork 358
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
UX improvement in relayer #541
Conversation
Codecov Report
@@ Coverage Diff @@
## master #541 +/- ##
=========================================
+ Coverage 13.6% 40.4% +26.7%
=========================================
Files 69 134 +65
Lines 3752 8487 +4735
Branches 1374 0 -1374
=========================================
+ Hits 513 3434 +2921
- Misses 2618 5053 +2435
+ Partials 621 0 -621
Continue to review full report at Codecov.
|
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.
Great improvements! Could you please also update the relayer operating instructions and the .toml example files to show how to control the logging.
Good point, that's something I overlooked! |
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.
Looks great! Left a couple suggestions but feel free to ignore if you feel it doesn't improve things much or can just come at a later point.
relayer-cli/src/commands/keys/add.rs
Outdated
Ok(r) => Output::with_success().with_result(json!(r)).exit(), | ||
Err(e) => Output::with_error() | ||
.with_result(json!(format!("{}", e))) | ||
.exit(), |
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 wonder if we could introduce a couple helpers to streamline such code a bit, eg.
Ok(r) => Output::with_success().with_result(json!(r)).exit(), | |
Err(e) => Output::with_error() | |
.with_result(json!(format!("{}", e))) | |
.exit(), | |
Ok(r) => Output::success(json!(r)).exit(), | |
Err(e) => Output::error(json!(format!("{}", e))).exit(), |
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.
Additionally, what do you think about changing the type of success
/error
/with_result
to
pub fn with_result(mut self, res: impl Serialize) -> Self {
self.result.push(serde_json::to_value(res).unwrap()); // same unwrap as in `json!` macro
self
}
which would result into
Ok(r) => Output::with_success().with_result(json!(r)).exit(), | |
Err(e) => Output::with_error() | |
.with_result(json!(format!("{}", e))) | |
.exit(), | |
Ok(r) => Output::success(r).exit(), | |
Err(e) => Output::error(format!("{}", e)).exit(), |
Having to call unwrap
on the result of serde_json::to_value
is unfortunate, but that's actually what the json!
macro does under the hood so perhaps it's okay?
Also note that this would still allow passing a custom JSON object with the json!
macro:
Output::success(json!({ "hello": "world" })).exit()
Reason is that json!
returns a Value
whose Serialize
instance produces the same Value
when given to to_value
.
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.
Looks good. I agree with Romain's comments, it would be nice to have that in this PR if it's a quick change. I will leave this up to you.
* Avoid unwraps in client txs. Better LightClient errors. * Avoided unwrap calls in tx raw commands * Tagged relayer RPC error with the culprit IP address. * Replaced start with v0 * Introduced logging levels. WIP: remove status_ calls * Updated the sample config file * Adapted keys commands to JSON output * Transition to tracing macros & JSON outputs * Changelog * Added operation instructions. * Simplified Output usage; h/t Romain's suggestions. * Better err message for client consensus query
Closes: #540
Closes: #536
Summary of changes:
tx raw
commands, removed theunwrap
call afterChainRuntime::spawn
and added error-handling code. This results in JSON error output in case the light client is not initialized, instead of panics:start
command now runs thev0
relayer; deleted thev0
commandlog_level
parameter in the relayer configuration file, so now we can enable error-only logging if we wish.info!
,debug!
etc macros, as replacements for abscissastatus_...
macros; both tracing and abscissa macros will adhere to thelog_level
parameter.For contributor use:
docs/
) and code comments.Files changed
in the Github PR explorer.