fix gasometer tracing #74
Conversation
|
Can you also add CI build for feature tracing? |
The command not working, so I create a new workspace. |
Why? What's the error? |
gasometer/src/lib.rs
Outdated
| #[cfg(feature = "tracing")] | ||
| pub fn snapshot(&self) -> Snapshot { | ||
| match self.inner.as_ref() { | ||
| Ok(inner) => Snapshot { |
There was a problem hiding this comment.
You can use ok().map(...).unwrap_or_default()
There was a problem hiding this comment.
Wait, but what's the meaning of Default here? Shouldn't we propagate the error?
There was a problem hiding this comment.
Please also fix this. Either change it to expect if you don't think errors will ever be returned, or properly handle the errors.
There was a problem hiding this comment.
I think should not handle errors in the event, otherwise, it will return a different result than without tracing
There was a problem hiding this comment.
Default means an error, but ignore it and let other checks deal with it
There was a problem hiding this comment.
Can you change the Snapshot struct to an enum representing either the current value, or a specific OutOfGas variant? You can also change all instances of Snapshot to Option<Snapshot>.
There was a problem hiding this comment.
Changed all instances of Snapshot to Option.
Does this satisfy the requirement?
Can you change the Snapshot struct to an enum representing either the current value, or a specific OutOfGas variant?
I'm not sure how to implement this
No error was returned, I feel it is a problem with the workspace. You can try this command. |
|
I can't reproduce the error at all. It works fine for me. |
Please try this: |
|
You missed some feature declarations. This has nothing to do with workspaces. |
This reverts commit 4a18e10.
| "evm-gasometer/tracing", | ||
| "evm-runtime/tracing" |
There was a problem hiding this comment.
I have modified it.
But I think the original way is better. People can enable tracing for evm/evm-gasometer/evm-runtime separately.
Closes: #73