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 (
|
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.
51bacb1 to
0fb08e1
Compare
|
ce9fb69 to
7575eb6
Compare
7575eb6 to
a8529b5
Compare
668af87 to
13e71ab
Compare
13e71ab to
efa6ad2
Compare
Codecov 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. |
| 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.
| 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
| 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)