-
Notifications
You must be signed in to change notification settings - Fork 698
refactor: some cleanups in process_tx_internal
#13003
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13003 +/- ##
==========================================
+ Coverage 69.69% 69.71% +0.01%
==========================================
Files 859 859
Lines 175995 176002 +7
Branches 175995 176002 +7
==========================================
+ Hits 122657 122697 +40
+ Misses 48176 48150 -26
+ Partials 5162 5155 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks!
self.forward_tx(&epoch_id, tx, signer)?; | ||
Ok(ProcessTxResponse::RequestRouted) | ||
return Ok(ProcessTxResponse::NoResponse); | ||
} | ||
// We are not tracking this shard, so there is no way to validate this tx. Just rerouting. | ||
self.forward_tx(&epoch_id, tx, signer).map(|()| ProcessTxResponse::RequestRouted) |
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.
The original version seems more readable to me, but I’m fine with both.
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 too think that the original version is easier to read however what I like about this version is that we make it explicit what forward_tx()
is returning in the happy path i.e. ()
. And if we ever make a change there, then the compiler will help us identify all the places where the changes need to be made.
looks like these changes are not correct. Nayduck tests are panicking and I suspect it is probably this line: nearcore/chain/chain/src/test_utils/kv_runtime.rs Line 1054 in 391d70b
|
- Remove some more `?` by using alternative means to get the data. - reduce indentation by inserting explicit `return`s. This makes it a bit easier to understand what the code is doing. Still I think there is room for improvement but bit by bit.
- Remove some more `?` by using alternative means to get the data. - reduce indentation by inserting explicit `return`s. This makes it a bit easier to understand what the code is doing. Still I think there is room for improvement but bit by bit.
?
by using alternative means to get the data.return
s. This makes it a bit easier to understand what the code is doing. Still I think there is room for improvement but bit by bit.