api: Add additional indexer info #6756
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the indexer state management across multiple files in the Hopr project. The modifications primarily focus on updating the type constraints for database operations, refactoring the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
db/sql/src/info.rs (2)
23-28: Add documentation for new struct fieldsConsider adding documentation comments for
latest_log_block_numberandlatest_log_checksumin theIndexerStateInfostruct to enhance code clarity and maintainability.
495-499: Evaluate the logging levelThe use of the
trace!macro for logging indexer state updates might be too verbose. Consider usingdebug!orinfo!for important operational logs to balance verbosity and usefulness.hoprd/rest-api/src/node.rs (1)
399-402: Add documentation for new response fieldsIn the
NodeInfoResponsestruct, consider adding comments or documentation for the new fieldsindexer_last_log_blockandindexer_last_log_checksumto improve code readability and API documentation.chain/indexer/src/block.rs (1)
Line range hint
85-90: Ensure atomicity in indexer state updatesThe comment mentions that indexing and marking logs as processed should be done in a single transaction but is currently difficult due to cross-database operations. Evaluate the possibility of restructuring to ensure atomic updates, which would enhance data integrity and reduce the risk of inconsistent states.
hopr/hopr-lib/src/lib.rs (1)
1112-1128: Implementation looks good but needs documentation.The implementation correctly combines indexer state info with log information, providing a more comprehensive view of the indexer state. However, since this is a breaking change in the return type, please add documentation to explain the new return type and its fields.
Add documentation for the method:
+ /// Returns the current state of the indexer, combining database state with the latest log information. + /// + /// # Returns + /// + /// Returns an `IndexerStateInfo` containing: + /// - The latest block number from the log + /// - The latest log checksum + /// - Additional state information from the database + /// + /// If no log exists, returns only the database state information. pub async fn get_indexer_state(&self) -> errors::Result<IndexerStateInfo> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
chain/indexer/src/block.rs(4 hunks)db/sql/src/info.rs(4 hunks)hopr/hopr-lib/src/lib.rs(2 hunks)hoprd/rest-api/src/node.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: Linter
🔇 Additional comments (5)
db/sql/src/info.rs (3)
7-7: Import oftracing::traceis necessaryThe
use tracing::trace;statement is appropriate as thetrace!macro is used later in the code for logging purposes.
471-475: Ensure all fields ofIndexerStateInfoare initializedIn the
get_indexer_state_infomethod, onlylatest_block_numberis set, whilelatest_log_block_numberandlatest_log_checksumare defaulted. Verify if these fields should be initialized with specific values from the database to prevent potential inconsistencies.
481-508: Consider updating all relevant fields inset_indexer_state_infoThe
set_indexer_state_infomethod only updateslast_indexed_block. Ensure thatlatest_log_block_numberandlatest_log_checksumare updated appropriately to maintain consistency in the indexer state.hoprd/rest-api/src/node.rs (1)
426-428: Verify completeness ofIndexerStateInfodataWhen calling
hopr.get_indexer_state(), ensure thatIndexerStateInfoincludes valid data forlatest_log_block_numberandlatest_log_checksum. Since these fields might be defaulted, verify that they are correctly retrieved and populated to prevent returning inaccurate information in the API response.chain/indexer/src/block.rs (1)
61-61: Updated type constraints are appropriateExtending the
Dbtype constraints to includeHoprDbInfoOperationsandHoprDbLogOperationsis necessary for the new methods used in the indexer.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
db/sql/src/info.rs (1)
493-497: Improve trace logging messageThe current trace message could be more descriptive.
Consider updating the trace message:
- "update block" + "updating last indexed block number"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
db/sql/src/info.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit
- GitHub Check: Linter
- GitHub Check: tests-smart-contracts
🔇 Additional comments (2)
db/sql/src/info.rs (2)
102-106: LGTM! Method signatures are well-defined.The new trait methods follow the established pattern of the codebase with proper async and transaction handling.
Line range hint
584-687: Add test coverage for new indexer state methodsThe new methods
get_indexer_state_infoandset_indexer_state_infolack test coverage. Consider adding tests to verify:
- Default state after initialization
- Setting and retrieving indexer state
- Edge cases and error conditions
Would you like me to help generate the test cases?
Sunsets existing indexer checksum info in the API's node info endpoint and adds additional values which reflect the new log implementation.
Fixes #6659