Uh oh!
There was an error while loading. Please reload this page.
refactor: Node interface, FullNode, LightNode, FullClient, LightClient - #1932
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
MSevey
left a comment
There was a problem hiding this comment.
Even though this is going on the feature branch for the exec_api, I think we should always have a passing CI.
Otherwise we get into the habit of merging failing CI and get ourselves into a hole.
If there are checks that need to be updated or removed for this feature branch we can do that, as long as we have a plan.
Uh oh!
There was an error while loading. Please reload this page.
51bacb1 to
0fb08e1Compare
|
ce9fb69 to
7575eb6Compare7575eb6 to
a8529b5Compare668af87 to
13e71abCompare13e71ab to
efa6ad2CompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## feature/exec_api #1932 +/- ##
===================================================
Coverage ? 39.42% ===================================================
Files ? 72 Lines ? 11250 Branches ? 0 ===================================================
Hits ? 4435 Misses ? 6197 Partials ? 618 ☔ View full report in Codecov by Sentry. |
tzdybal
left a comment
There was a problem hiding this comment.
LGTM.
A lot of cleanup. We need to reintroduce tests (maybe rewrite them to some extend) ASAP (may be follow-up PR).
| ChainID: h.ChainID(), | ||
| } | ||
| return Hash(abciHeader.Hash()) | ||
| //abciHeader := cmtypes.Header{ |
There was a problem hiding this comment.
How the godoc of this method is inaccurate - it's not ABCI-compatible anymore.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| initialHeight := uint64(m.genesis.InitialHeight) //nolint:gosec | ||
| initialHeight := m.genesis.InitialHeight //nolint:gosec |
There was a problem hiding this comment.
ideally we'd have a defensive check here as InitialHeight might be -1:
if initial < 0 {
return fmt.Errorf("initial height cannot be negative: %d", m.genesis.InitialHeight)
}
There was a problem hiding this comment.
it's of type uint64
tzdybal
left a comment
There was a problem hiding this comment.
LGTM. My comment is optional and might be resolved in follow-up PR.
| require.NotNil(executor) | ||
| // Test GetTxs | ||
| txs, err := executor.GetTxs(ctx) |
There was a problem hiding this comment.
Is there any assertion on tx's? for example, that there are some?
There was a problem hiding this comment.
yes, will go in followup
There was a problem hiding this comment.
Please make a issue for this
Cleaning up rollkit, in preparation for execution api (#1896#1897#1898#1899#1900)