Initial FastNear indexer integration #321
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request implements multiple updates across various workflow files and configuration settings. Key changes include modifications to GitHub Actions workflows to refine trigger conditions based on file paths, updates to Docker configurations, and enhancements to Rust dependency management. New structs and error types are introduced in the Rust codebase, alongside adjustments to existing functions to improve error handling and logging. The changes aim to streamline the build and testing processes while correcting inconsistencies in naming and configurations. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
Fly-Style
force-pushed
the
fastnear-indexer
branch
from
7b37ea0 to
9a78477
Compare
Fly-Style
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
🧹 Outside diff range and nitpick comments (38)
.github/workflows/docs-test.yml (1)
Line range hint
1-24: Workflow requires essential improvementsCritical deficiencies identified in the workflow configuration:
- No explicit error handling or reporting
- No validation of generated documentation
- No artifact retention for failed builds
Apply these improvements:
name: Test deployment on: pull_request: branches: - main paths: - 'docs/**' jobs: test-deploy: name: Test docs deployment runs-on: ubuntu-latest + continue-on-error: false steps: - uses: actions/checkout@v4 with: fetch-depth: 0 - uses: actions/setup-node@v4 with: node-version: 18 cache: npm cache-dependency-path: docs - name: Install dependencies run: npm ci working-directory: docs + - name: Test build website run: npm run build working-directory: docs + + - name: Validate generated documentation + run: | + if [ ! -f "build/index.html" ]; then + echo "Error: Documentation build failed - index.html not found" + exit 1 + fi + working-directory: docs + + - name: Upload documentation artifact + if: failure() + uses: actions/upload-artifact@v3 + with: + name: docs-build + path: docs/build/ + retention-days: 5.github/workflows/contracts-tests.yml (2)
Line range hint
16-16: 5-minute timeout is dangerously optimisticA 5-minute timeout for contract tests is inadequate. Complex contracts or large test suites could easily exceed this limit, especially when running with maximum verbosity (-vvv).
Recommend increasing to at least 15 minutes:
- timeout-minutes: 5 + timeout-minutes: 15
Line range hint
31-33: Using Foundry nightly is a reliability riskNightly builds can introduce unexpected breaking changes. Pin to a specific version for reproducible builds.
- version: nightly + version: 0.2.0 # or another stable versionindexer/FastIndexer.dockerfile (2)
45-45: Entrypoint lacks explicit shell specificationThe entrypoint should explicitly specify the shell to ensure consistent behaviour across different environments.
-ENTRYPOINT [ "./entrypoint.sh" ] +ENTRYPOINT [ "/bin/sh", "./entrypoint.sh" ]
1-45: Missing crucial security configurationsThe Dockerfile lacks several security best practices that are essential for production deployments.
Add the following security configurations:
- Create and use a non-root user
- Set appropriate file permissions
- Configure security options
+# In runtime stage after installing dependencies +RUN groupadd -r indexer && \ + useradd -r -g indexer -s /bin/false indexer && \ + chown -R indexer:indexer /indexer-app + +USER indexer + +# Add security options +SECURITY_OPTS --security-opt=no-new-privileges:true \ + --cap-drop=ALLindexer/README.md (1)
26-36: Consider architectural resilience.The tight coupling with FastNear service introduces a single point of failure. Consider implementing:
- Fallback mechanism to standard NEAR RPC endpoints
- Circuit breaker pattern for graceful degradation
- Health check endpoints for monitoring FastNear availability
Would you like assistance in implementing these resilience patterns?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...fetch NEAR blocks more efficiently. The--features use_fastnearflag enables the FastNEAR...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🪛 Markdownlint (0.35.0)
31-31: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
indexer/Dockerfile (1)
22-24: Remove unprofessional commentary and document the caching strategy properlyThe term "hacky" is unprofessional. This is a legitimate Docker layer caching strategy that deserves proper documentation.
Replace with:
-# Hacky approach to cache dependencies +# Pre-build dependencies for layer caching +# Create a dummy main.rs to compile dependencies without application codeindexer/Cargo.toml (1)
43-44: Document the feature flag purposeThe
use_fastnearfeature flag lacks documentation explaining its purpose and impact.Add a comment explaining when this feature should be enabled:
[features] + # Enable this feature to use the FastNear indexer implementation use_fastnear = []tests/e2e/e2e_tests/tests/ctr_availability_test.rs (5)
Line range hint
18-23: Empty expect messages are unacceptable.The empty expect message
expect("")provides zero context for test failures. This is a critical oversight in error handling.- rabbitmq.start().await.expect(""); + rabbitmq.start().await.expect("Failed to start RabbitMQ container - ensure Docker daemon is running and port is available");
Line range hint
18-85: Missing container health checks.Starting containers without health checks is insufficient. Each container should verify its readiness before the test proceeds.
Example implementation for RabbitMQ:
async fn check_rabbitmq_available() { let rabbitmq = rabbitmq(); let container = rabbitmq.start().await.expect("Failed to start RabbitMQ container"); // Add health check let health_check = async { let mut attempts = 0; while attempts < 30 { if let Ok(_) = container.check_health().await { return Ok(()); } tokio::time::sleep(Duration::from_secs(1)).await; attempts += 1; } Err("Container failed health check") }; health_check.await.expect("RabbitMQ failed to become healthy"); }
Line range hint
1-85: Missing cleanup verification.While the file comments mention automatic cleanup, there's no verification that containers are actually removed after tests, especially after failures.
Add a cleanup verification helper:
async fn verify_cleanup(container_name: &str) { let docker = bollard::Docker::connect_with_local_defaults() .expect("Failed to connect to Docker daemon"); let filters = HashMap::from([ ("name", vec![container_name]) ]); let containers = docker.list_containers(Some(ListContainersOptions { filters, ..Default::default() })) .await .expect("Failed to list containers"); assert!(containers.is_empty(), "Container {} was not cleaned up", container_name); }
Hardcoded relative path requires immediate correction
The file exists at
config-files/operator1-docker-compose.anvil.yaml, confirming that the current relative path../../../config-files/operator1-docker-compose.anvil.yamlis unnecessarily complex and brittle. This should be replaced with a path relative to the workspace root or use environment variables/configuration to specify the path.
- Replace
"../../../config-files/operator1-docker-compose.anvil.yaml"with"config-files/operator1-docker-compose.anvil.yaml"or implement a more robust path resolution mechanism.🔗 Analysis chain
Line range hint
77-82: Hardcoded config path is brittle.The operator test uses a hardcoded relative path
"../../../config-files/operator1-docker-compose.anvil.yaml". This is fragile and will break if the test file moves.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and usage of the config file fd operator1-docker-compose.anvil.yamlLength of output: 89
Port 8546 is used without availability checks, posing a reliability risk.
The codebase directly uses port 8546 for the Anvil rollup node without first verifying its availability. This oversight can lead to silent failures when:
- Another process is already using port 8546
- The port is blocked by firewall rules
- The port is reserved by the system
Required actions:
- Implement port availability check before starting the rollup node
- Add fallback logic to find an available port if 8546 is occupied
- Add proper error handling and user feedback when port binding fails
🔗 Analysis chain
Line range hint
39-44: Hardcoded ports require validation.Port 8546 is hardcoded without any port availability check. This will fail silently if the port is in use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if critical ports are defined elsewhere in the codebase rg -g '!target/' '8546|3030' .Length of output: 116977
Script:
#!/bin/bash # Check if port availability is checked before using port 8546 rg -A 2 "8546" . --type rustLength of output: 810
Script:
#!/bin/bash # Search for port availability checks in the codebase rg -B 2 -A 2 "is_available|check.*port|port.*check|TcpListener::bind" .Length of output: 1307
Script:
#!/bin/bash # Check the implementation of anvil_rollup_node to see if there's any port availability check rg -A 10 "fn anvil_rollup_node" .Length of output: 899
.github/workflows/compose-test.yml (1)
131-131: Add newline at end of fileThe file is missing a newline character at the end, which is a YAML best practice.
run: docker-compose -f ./docker-compose.yml down +🧰 Tools
🪛 yamllint (1.35.1)
[error] 131-131: no new line character at the end of file
(new-line-at-end-of-file)
docker-compose.yml (4)
Line range hint
239-350: CRITICAL: Exposed credentials in plain textMultiple security vulnerabilities detected:
- Hard-coded ECDSA private key in aggregator service
- Plain text BLS and ECDSA key passwords in operator services
- Admin credentials in Grafana configuration
These credentials must be moved to a secure secrets management solution.
Implement one of these solutions:
- Docker secrets
- Environment file with proper access controls
- External secrets management service (HashiCorp Vault, AWS Secrets Manager)
Line range hint
239-350: Standardise service dependency conditionsInconsistent dependency conditions detected:
- operator1 depends on operator0-health (health check)
- operator0 depends on aggregator (service_started)
- indexer depends on multiple services with different conditions
This inconsistency could lead to race conditions and unreliable service startup.
Standardise to use health checks consistently:
operator0: depends_on: aggregator: - condition: service_started + condition: service_healthy
Line range hint
1-7: Implement network segmentationCurrent configuration places all services on a single bridge network, violating the principle of least privilege. Services should be grouped into separate networks based on their communication requirements.
Consider implementing:
- Frontend network for Grafana
- Metrics network for Prometheus
- Backend network for core services
- Database network for persistent storage
Health check configurations require standardisation with clear justification for deviations
The verification confirms inconsistent health check configurations across services:
- Most services: 15s interval, 10s timeout
- RabbitMQ: 1s interval, 3s timeout
- One service (port 8547): 2s interval, 30s timeout
These variations lack clear justification and could lead to inconsistent monitoring behaviour. The RabbitMQ configuration might be justified due to its critical message broker role, but the service on port 8547 requires explanation for its extended timeout.
🔗 Analysis chain
Line range hint
239-248: Standardise health check configurations across servicesThe health check configuration for the aggregator-health service differs from the established pattern seen in other services. Most services use a 15s interval and 10s timeout, while rollup1-anvil inexplicably uses 2s interval and 30s timeout. This inconsistency requires justification.
Run this script to verify health check configurations across services:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Extract and compare health check configurations rg -A 3 'healthcheck:' docker-compose.ymlLength of output: 839
tests/integration/integration_test.go (1)
Line range hint
1-999: Test structure needs improvementThe test file has several structural issues that make it hard to maintain and understand:
- The test is monolithic with multiple responsibilities
- Setup code is complex and mixed with test logic
- Cleanup is not guaranteed in case of panics
- Magic numbers and constants are scattered throughout
Consider these improvements:
- Break down the test into smaller, focused test cases
- Move setup code to a separate test fixture package
- Use
deferfor cleanup operations- Extract configuration constants to a dedicated constants file
- Add proper test documentation explaining the test scenarios
Example structure:
type TestFixture struct { mainnetAnvil *utils.AnvilInstance rollupAnvils []*utils.AnvilInstance rabbitMq *rabbitmq.RabbitMQContainer // ... other fields } func NewTestFixture(t *testing.T) *TestFixture { // Setup code here } func (f *TestFixture) Cleanup(t *testing.T) { // Cleanup code here } func TestOperatorSetUpdate(t *testing.T) { fixture := NewTestFixture(t) defer fixture.Cleanup(t) // Test specific logic here } func TestStateRootUpdate(t *testing.T) { fixture := NewTestFixture(t) defer fixture.Cleanup(t) // Test specific logic here }indexer/src/types.rs (3)
16-27: MissingSerializeandDeserializederivations.
PartialCandidateDataandPartialCandidateDataWithBlockTxHashlackserde::Serializeandserde::Deserializederivations. Add them to ensure consistency and enable serialization.Apply this diff to add the derivations:
#[derive(Clone, Debug)] +#[derive(serde::Serialize, serde::Deserialize)] pub(crate) struct PartialCandidateData { pub rollup_id: u32, pub payloads: Vec<Vec<u8>>, } #[derive(Clone, Debug)] +#[derive(serde::Serialize, serde::Deserialize)] pub(crate) struct PartialCandidateDataWithBlockTxHash { pub rollup_id: u32, pub payloads: Vec<Vec<u8>>, pub tx_hash: CryptoHash, pub block_hash: CryptoHash, }
16-27: Inconsistent visibility modifiers among structs.Some structs are marked
pub(crate)while others arepub. Ensure visibility modifiers are consistent and appropriate for your module's design.
16-65: Add documentation comments to public interfaces.Public structs and enums lack
///documentation comments. Add them to improve code clarity and facilitate maintenance.indexer/src/main.rs (1)
105-105: Inconsistent logging target usage for startup message.Previously, the constant
INDEXERwas used as the logging target. Changing it toINDEXER_NAMESPACEmay lead to inconsistencies in log filtering and grouping.Consider reverting to the original logging target or ensuring that all logging statements use
INDEXER_NAMESPACEconsistently throughout the codebase.indexer/src/fastnear_indexer.rs (2)
133-135: Eliminate unnecessary cloning ofcandidate_data.Cloning
candidate_datais inefficient and consumes extra memory. Iterate over references to the payloads instead to improve performance.Apply the following modification:
async fn send(candidate_data: &PartialCandidateDataWithBlockTxHash, sender: &Sender<PublishData>) -> Result<(), Error> { trace!(target: "fastnear_indexer", "Sending candidate data: {:?}", candidate_data); - for data in candidate_data.clone().payloads { + for data in &candidate_data.payloads { let publish_data = PublishData { publish_options: PublishOptions { routing_key: get_routing_key(candidate_data.rollup_id), ..PublishOptions::default() }, cx: PublisherContext { block_hash: candidate_data.block_hash, }, payload: PublishPayload { transaction_id: candidate_data.tx_hash, - data, + data: data.clone(), }, }; sender.send(publish_data).await? } Ok(()) }
105-105: Avoid cloning the block before sending over the channel.Cloning
blockis unnecessary if ownership can be transferred. This reduces memory usage and improves efficiency.Modify the send operation to move the block:
- if block_sender.send(block.clone()).await.is_err() { + if block_sender.send(block).await.is_err() {indexer/src/block_listener.rs (9)
Line range hint
37-44: MakeTransactionWithRollupIdpublic for broader accessibilityThe struct
TransactionWithRollupIdis currently private. If it needs to be accessed outside of this module, declare it aspub(crate)orpubto ensure proper visibility.
Line range hint
46-49: Simplify nestedtransactionfield access for clarityThe line:
let actions = &transaction.transaction.transaction.actions;has multiple levels of
transaction, which is confusing. Refactor to reduce nesting or rename fields to enhance readability.
Line range hint
119-139: Incomplete error handling influshfunctionIn the
flushmethod, when sending candidate data fails, the error is ignored, and the method returnsfalsewithout proper handling:// TODO: return TrySendError instead Err(_) => return false,Implement comprehensive error handling to address potential issues during data transmission.
Line range hint
164-198: Potential deadlock due to mutex usage inprocess_streamHolding the mutex lock on
queue_protectedwhile performing I/O operations or awaiting asynchronous tasks can lead to deadlocks. Re-evaluate the lock's scope and limit its usage to only when accessing shared data.
Line range hint
230-255: Inefficient loop in candidate data processingThe manual iteration over
candidates_data:while let Some(candidate) = iter.next() { // ... }can be replaced with a for-loop or iterators to improve code efficiency and readability.
Line range hint
378-385: Direct comparison of floating-point numbers in testsUsing
assert_eq!for floating-point numbers can cause flaky tests due to precision errors. Use approximate equality methods or tolerate a small epsilon value.
Line range hint
400-405: Unnecessary cloning ofvalid_transactionin testsCloning
valid_transactionis redundant since it's not modified. Remove the clone to optimize memory usage.
Line range hint
510-515: Use of magic number intest_shutdownThe condition
if i == 5uses a magic number5. Define a constant or add a comment to explain the significance for better maintainability.
Line range hint
530-550: Inefficient file reading inStreamerMessagesLoader::load_messagesCollecting directory entries into a
Vec<PathBuf>before processing is unnecessary. Iterate over the entries directly to streamline file loading.tests/integration/fastnear_test.go (4)
367-368: Avoid setting environment variables within the codeModifying environment variables like
OPERATOR_BLS_KEY_PASSWORDandOPERATOR_ECDSA_KEY_PASSWORDinside the code can lead to side effects and makes the code less predictable.Consider removing these lines and setting the environment variables outside the application, such as in the test setup or via a configuration management tool.
- os.Setenv("OPERATOR_BLS_KEY_PASSWORD", "") - os.Setenv("OPERATOR_ECDSA_KEY_PASSWORD", "")
285-285: Check for potential goroutine leaksWhen starting goroutines within tests (
go operator.Start(ctx)andgo agg.Start(ctx)), ensure they are properly terminated to prevent resource leaks.
- Confirm that the
ctxpassed to these functions is adequately cancelled when the test completes.- Use
deferstatements ort.Cleanupto manage goroutine lifecycles.Also applies to: 323-323
687-710: Consolidate duplicate HTTP request handling codeThe functions
getStateRootUpdateAggregationandgetOperatorSetUpdateAggregationhave similar logic for making HTTP GET requests and handling responses.Abstract the common code into a single helper function to reduce duplication and improve maintainability.
Also applies to: 712-737
178-263: Add error checks after creating the networkThere is no check for errors when removing the network in the cleanup function if network creation failed.
Ensure that resources are only cleaned up if they were successfully created:
- if err := net.Remove(containersCtx); err != nil { + if net != nil { + if err := net.Remove(containersCtx); err != nil { t.Fatalf("Error removing network: %s", err.Error()) } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.github/workflows/compose-fastnear-test.yml(1 hunks).github/workflows/compose-test.yml(6 hunks).github/workflows/contracts-tests.yml(1 hunks).github/workflows/docs-test.yml(1 hunks).github/workflows/unit-tests.yml(2 hunks)Cargo.toml(1 hunks)Makefile(3 hunks)docker-compose-fastnear.yml(1 hunks)docker-compose.yml(1 hunks)indexer/Cargo.toml(1 hunks)indexer/Dockerfile(2 hunks)indexer/FastIndexer.dockerfile(1 hunks)indexer/README.md(1 hunks)indexer/src/block_listener.rs(1 hunks)indexer/src/errors.rs(2 hunks)indexer/src/fastnear_indexer.rs(1 hunks)indexer/src/main.rs(4 hunks)indexer/src/metrics.rs(1 hunks)indexer/src/types.rs(3 hunks)indexer/test_data/config/example.yaml(1 hunks)rust-toolchain.toml(1 hunks)tests/e2e/e2e_tests/tests/containers.rs(0 hunks)tests/e2e/e2e_tests/tests/ctr_availability_test.rs(1 hunks)tests/integration/fastnear_test.go(1 hunks)tests/integration/integration_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/e2e_tests/tests/containers.rs
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- rust-toolchain.toml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/compose-fastnear-test.yml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
111-111: shellcheck reported issue in this script: SC2086:info:11:57: Double quote to prevent globbing and word splitting
(shellcheck)
111-111: shellcheck reported issue in this script: SC2086:info:14:58: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/compose-fastnear-test.yml
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/compose-test.yml
[error] 131-131: no new line character at the end of file
(new-line-at-end-of-file)
indexer/test_data/config/example.yaml
[error] 37-37: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.21.2)
docker-compose-fastnear.yml
240-240: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
271-271: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
272-272: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/integration/fastnear_test.go
454-454: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
511-511: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
543-543: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
indexer/README.md
[uncategorized] ~34-~34: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...fetch NEAR blocks more efficiently. The --features use_fastnear flag enables the FastNEAR...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🪛 Markdownlint (0.35.0)
indexer/README.md
31-31: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
🔇 Additional comments (10)
.github/workflows/docs-test.yml (1)
7-8: Verify comprehensive documentation coverage
The paths filter docs/** assumes all documentation resides in the docs directory. This might miss documentation-related changes in:
- README files
- API documentation in source code
- Markdown files in other directories
.github/workflows/contracts-tests.yml (1)
Line range hint 43-44: Test command excludes FFI contracts without justification
The --no-match-contract FFI flag excludes FFI contracts from testing. This could leave critical functionality untested.
Let's check for FFI contract usage:
indexer/Cargo.toml (1)
30-34: Verify NEAR framework compatibility
All NEAR dependencies are pinned to git commit a83c18490cf4. This is good for consistency, but:
indexer/src/errors.rs (1)
68-68: The Result type alias follows standard Rust conventions.
indexer/src/metrics.rs (2)
6-6: Documentation must be updated to reflect the namespace change
Making INDEXER_NAMESPACE public and changing its value requires documentation updates. This change affects metric collection configuration and dashboards.
6-6:
CRITICAL: Verify impact on existing metrics collection
The namespace change from "sffl_indexer" to "nffl_indexer" will affect all metrics collection. This modification requires immediate attention to prevent monitoring disruption.
✅ Verification successful
Let me examine the monitoring configurations to determine if there are any dependencies on the old namespace and assess the potential impact of this change.
Namespace change is safe to proceed
The search results show no references to the old "sffl_indexer" namespace in any of the monitoring configurations or metric endpoints. The metrics configuration is primarily handled through IP:port specifications in docker-compose files and yaml configurations, which are independent of the Rust namespace change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old namespace
rg "sffl_indexer" --type yaml --type rust
# Look for potential monitoring configuration files
fd -e yaml -e yml . | xargs grep -l "prometheus\|metrics\|monitoring"
Length of output: 818
Script:
#!/bin/bash
# Check the content of prometheus configuration files for the old namespace
rg -A 5 -B 5 "sffl_indexer" prometheus/prometheus.yml setup/*/prometheus/prometheus.yml
# Check monitoring configurations in docker compose files
rg -A 5 -B 5 "metrics" docker-compose*.yml setup/*/docker-compose.yml
# Check if there are any metric-related configurations in yaml files
rg -A 5 -B 5 "metrics:" config-files/*.yaml setup/*/config/*.yaml
Length of output: 12632
.github/workflows/compose-test.yml (1)
90-90: Verify consistent container naming across all configurations
The container name has been corrected from "agreator" to "aggregator". Let's ensure this correction is applied consistently across all configuration files.
Also applies to: 113-113
✅ Verification successful
Container naming is consistent across configurations
The search results confirm that there are no remaining instances of the "agreator" typo in any YAML configurations. All references correctly use "aggregator" in container names, configuration parameters, and service definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the typo
rg -i "agreator" --type yaml
Length of output: 28
Script:
#!/bin/bash
# Search for both old and new container names in yaml files
rg -i "nffl.*health" --type yaml
# Also check for any variations of aggregator/agreator in docker-compose files
fd -e yml -e yaml | xargs grep -i "aggregator\|agreator"
Length of output: 4556
Makefile (1)
145-147:
Disabled test without justification
The registration test has been commented out without explanation. This reduces test coverage and could mask potential issues.
Either:
- Restore the test if it's still relevant
- Remove it if it's obsolete
- Document why it's disabled with a TODO comment
Additionally, the new fastnear test lacks documentation about its purpose and requirements.
indexer/src/main.rs (2)
113-113: Ensure new logging filters are correctly applied.
Adding fastnear_indexer=info to the logging filter is appropriate. Verify that all relevant modules are included and that the logging levels are set as intended.
118-118: Potential issue with default home directory resolution.
Ensure that the change from pattern matching to using unwrap_or_else does not alter the behaviour when home_dir is not provided. Confirm that near_indexer::get_default_home correctly returns the intended default directory.
Run the following script to confirm the default home directory:
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
indexer/Cargo.toml (1)
43-45: Document feature flags usageNew features
use_fastnearandit_testshave been added without documentation. Given that integration tests are mentioned as work in progress in the PR description, we need clarity on these features' usage.Add feature documentation in the Cargo.toml:
[features] +# Enable FastNear indexer integration use_fastnear = [] +# Enable integration tests (currently WIP) it_tests = [].github/workflows/integration-tests.yml (1)
80-83: Address integration test dependenciesAccording to the PR objectives, there are "complexities associated with the nearcore setup". The workflow doesn't explicitly handle these dependencies.
Consider:
- Add explicit checks for nearcore dependencies
- Include setup verification steps
- Document required environment configuration
- Add timeout parameters specific to FastNear tests
indexer/src/main.rs (1)
105-113: Logging filter string requires refactoring.The logging filter string is becoming difficult to maintain. Consider extracting it to a configuration file or constant.
Extract the filter string to a constant:
+const DEFAULT_LOG_FILTER: &str = concat!( + "nearcore=info,publisher=info,indexer=info,candidates_validator=info,", + "metrics=info,tokio_reactor=info,near=info,stats=info,telemetry=info,", + "near-performance-metrics=info,fastnear_indexer=info" +); - let env_filter = near_o11y::tracing_subscriber::EnvFilter::new( - "nearcore=info,publisher=info,indexer=info,candidates_validator=info,\ - metrics=info,tokio_reactor=info,near=info,stats=info,telemetry=info,\ - near-performance-metrics=info,fastnear_indexer=info", - ); + let env_filter = near_o11y::tracing_subscriber::EnvFilter::new(DEFAULT_LOG_FILTER);indexer/src/configs.rs (1)
56-57: Fix style inconsistencies
- Missing trailing comma after the new field
- Inconsistent documentation style compared to other fields
Apply this diff to fix the style issues:
#[clap(long, default_value = "256")] - pub channel_width: usize + pub channel_width: usize,indexer/src/fastnear_indexer.rs (1)
239-239: Eliminate hardcodedchannel_widthvalueUsing a magic number (
128) forchannel_widthin the test reduces flexibility and readability. Definechannel_widthas a constant or pass it as a parameter to allow for easier adjustments and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/integration-tests.yml(1 hunks)indexer/Cargo.toml(1 hunks)indexer/src/configs.rs(1 hunks)indexer/src/fastnear_indexer.rs(1 hunks)indexer/src/main.rs(4 hunks)
🧰 Additional context used
📓 Learnings (2)
indexer/Cargo.toml (1)
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/Cargo.toml:37-38
Timestamp: 2024-11-28T13:42:30.249Z
Learning: When reviewing `indexer/Cargo.toml`, note that moving dependencies to the workspace will be handled separately. Do not suggest moving dependencies to the workspace in the current pull request.
indexer/src/main.rs (2)
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/src/main.rs:43-50
Timestamp: 2024-11-28T13:49:02.995Z
Learning: In `indexer/src/main.rs`, using `if cfg!(feature = "use_fastnear")` is intended and acceptable; do not suggest replacing it with conditional compilation attributes in future reviews.
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/src/main.rs:100-100
Timestamp: 2024-11-28T13:48:31.475Z
Learning: `Error::AnyhowError` requires the `anyhow!` macro as an argument to produce an `anyhow::Error`.
🔇 Additional comments (8)
.github/workflows/integration-tests.yml (1)
80-82:
Clarify the distinction between duplicate test features
Both Rust test steps use identical feature flags --features it-tests but run in different directories. This creates ambiguity about test scope and purpose.
Let's verify the feature definitions in both locations:
Consider:
- Using distinct feature flags for each test suite
- Adding conditional execution based on changed files
- Documenting the purpose of each test suite in comments
indexer/src/main.rs (3)
10-11: Module structure is acceptable.
The new fastnear_indexer module and its imports are properly integrated into the existing codebase.
Also applies to: 22-22
39-41: Metrics configuration simplified appropriately.
The change to is_some() is more concise than pattern matching for this use case.
100-100: Error handling implementation is correct.
The use of anyhow! macro with Error::AnyhowError follows the established pattern.
indexer/src/configs.rs (1)
56-57: Verify channel_width usage and performance impact
The addition of channel_width parameter requires verification:
- Confirm its usage in FastNear indexer implementation
- Verify performance implications with different values
- Consider adding metrics for channel utilisation
✅ Verification successful
Let me analyze the channel usage patterns and any performance considerations in the tests before making a final determination.
Channel width parameter implementation is adequate
The channel_width parameter is correctly integrated:
- Used in FastNearIndexer to configure two tokio mpsc channels for block and publish operations
- Default value of 256 is reasonable for async message processing
- No performance metrics needed as tokio channels are self-regulating with back-pressure
The implementation follows standard patterns for async Rust channel usage. Adding metrics would be superfluous as channel capacity issues manifest through back-pressure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for channel_width usage in the codebase
echo "Searching for channel_width usage:"
rg -p "channel_width" --type rust
# Search for potential performance metrics
echo "Checking for related metrics:"
rg -p "metrics|performance|channel" src/
Length of output: 822
Script:
#!/bin/bash
# Search for channel usage in the indexer implementation
ast-grep --pattern 'mpsc::channel($$$)' indexer/src/fastnear_indexer.rs
# Check for any performance-related comments or documentation
rg -p "performance|throughput|channel" indexer/
# Look for any tests that might validate channel behavior
fd -e rs test -x rg -p "channel_width|mpsc::channel" {}
Length of output: 2144
indexer/src/fastnear_indexer.rs (3)
176-183:
Incorrect handling of successful execution statuses
The is_successful_execution function only considers SuccessValue with an empty value as successful. This exclusion overlooks successful executions that return non-empty values or SuccessReceiptId. This oversight can lead to missing valid data.
Apply the following diff to correctly handle all success variants:
fn is_successful_execution(receipt_execution_outcome: &IndexerExecutionOutcomeWithReceiptAndTxHash) -> bool {
let is_successful = matches!(
receipt_execution_outcome.execution_outcome.outcome.status,
- ExecutionStatusView::SuccessValue(ref value) if value.is_empty()
+ ExecutionStatusView::SuccessValue(_) | ExecutionStatusView::SuccessReceiptId(_)
);
trace!(FASTNEAR_INDEXER, "Execution successful: {}", is_successful);
is_successful
}93-104:
Avoid skipping processing when tx_hash is missing
By requiring tx_hash to be Some, the code disregards cases where tx_hash is None, potentially missing relevant data. This could result in incomplete processing of valid receipts.
Consider adjusting the condition:
- if let (Some(partial_data), Some(tx_hash)) =
- (partial_candidate_data, receipt_execution_outcome.tx_hash)
+ if let Some(partial_data) = partial_candidate_data {
+ let tx_hash = receipt_execution_outcome.tx_hash.unwrap_or_default();Ensure that using a default tx_hash is appropriate and won't introduce inconsistencies.
117-131: 🛠️ Refactor suggestion
Implement exponential backoff to handle fetch failures
Currently, after a fetch failure, the loop waits for a fixed 1-second delay before retrying. This can cause rapid retries during persistent failures, leading to unnecessary load and potential rate limiting. An exponential backoff strategy is essential to mitigate these risks.
Apply the following changes:
tokio::spawn(async move {
+ let mut failure_count = 0;
loop {
match Self::fetch_latest_block(&client).await {
Ok(block) => {
if block_sender.send(block.clone()).await.is_err() {
error!(FASTNEAR_INDEXER, "Failed to send block to channel");
break;
}
info!(FASTNEAR_INDEXER, "Successfully fetched and sent latest block with id: {}", block.block.header.height);
+ failure_count = 0; // Reset failure count on success
}
Err(e) => {
error!(FASTNEAR_INDEXER, "Error fetching latest block: {:?}", e);
+ failure_count += 1;
+ let delay = std::time::Duration::from_secs(1 << std::cmp::min(failure_count, 5));
+ tokio::time::sleep(delay).await;
+ continue;
}
}
- tokio::time::sleep(std::time::Duration::from_secs(1)).await;
+ tokio::time::sleep(std::time::Duration::from_secs(1)).await; // Regular interval between fetches
}
});
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
indexer/src/fastnear_indexer.rs(1 hunks)
🔇 Additional comments (2)
indexer/src/fastnear_indexer.rs (2)
177-184:
Broaden success criteria to include all valid success states
The current implementation only considers empty success values as successful executions, potentially missing valid successful states.
This issue was previously identified. The implementation should be updated to include all success variants as suggested in the previous review.
117-132: 🛠️ Refactor suggestion
Implement proper error handling with exponential backoff
The current implementation continues to hammer the endpoint even when errors occur. This could lead to rate limiting or service degradation.
Implement exponential backoff:
tokio::spawn(async move {
+ let mut backoff = 1;
+ let max_backoff = 32;
loop {
match Self::fetch_latest_block(&client).await {
Ok(block) => {
let block_height = block.block.header.height;
if block_sender.send(block).await.is_err() {
error!(FASTNEAR_INDEXER, "Failed to send block to channel");
break;
}
info!(FASTNEAR_INDEXER, "Successfully fetched and sent latest block with id: {}", block_height);
+ backoff = 1;
}
- Err(e) => error!(FASTNEAR_INDEXER, "Error fetching latest block: {:?}", e),
+ Err(e) => {
+ error!(FASTNEAR_INDEXER, "Error fetching latest block: {:?}", e);
+ backoff = std::cmp::min(backoff * 2, max_backoff);
+ }
}
- tokio::time::sleep(std::time::Duration::from_secs(1)).await;
+ tokio::time::sleep(std::time::Duration::from_secs(backoff)).await;
}
});Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
indexer/src/main.rs (1)
58-72: High coupling between indexer components.The implementation tightly couples the
IndexerWrapper,CandidatesValidator, andRmqPublisher, making the system less maintainable and harder to test.Consider implementing a message bus or event-driven architecture to decouple these components. This would allow for:
- Independent testing of each component
- Easier maintenance and modifications
- Better error isolation
indexer/src/fastnear_indexer.rs (1)
241-241: Avoid magic numbers; makechannel_widthconfigurable or use a constantUsing a hardcoded value like
128forchannel_widthreduces code clarity and flexibility. Define it as a constant or make it configurable to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
indexer/src/fastnear_indexer.rs(1 hunks)indexer/src/main.rs(4 hunks)
🧰 Additional context used
📓 Learnings (1)
indexer/src/main.rs (2)
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/src/main.rs:43-50
Timestamp: 2024-11-28T13:49:02.995Z
Learning: In `indexer/src/main.rs`, using `if cfg!(feature = "use_fastnear")` is intended and acceptable; do not suggest replacing it with conditional compilation attributes in future reviews.
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/src/main.rs:100-100
Timestamp: 2024-11-28T13:48:31.475Z
Learning: `Error::AnyhowError` requires the `anyhow!` macro as an argument to produce an `anyhow::Error`.
🔇 Additional comments (10)
indexer/src/main.rs (6)
10-11: Module structure and imports are properly organised.
The new module declaration and imports align with the FastNear indexer integration objectives.
Also applies to: 22-22
43-50: Channel width parameter requires validation.
The channel_width parameter is passed directly to FastNearIndexer::new without bounds checking. This remains a potential risk for system stability.
99-99: Error handling implementation is correct.
The use of anyhow::anyhow! macro with Error::AnyhowError follows the expected pattern.
104-104: Logging configuration properly updated.
The logging target and filter changes appropriately accommodate the new FastNear indexer module.
Also applies to: 112-112
117-117: Home directory handling follows best practices.
The use of unwrap_or_else provides a safe fallback mechanism for the home directory.
74-74:
Inadequate error handling for block handle.
The block handle's error handling relies solely on await without proper handling of cancellation scenarios.
Implement comprehensive error handling that accounts for both cancellation and panic scenarios:
- Ok::<_, Error>(block_handle.await?)
+ match block_handle.await {
+ Ok(result) => Ok(result),
+ Err(e) if e.is_cancelled() => {
+ error!(target: INDEXER, "Block processing cancelled: {}", e);
+ Err(Error::BlockProcessingCancelled(e))
+ }
+ Err(e) => {
+ error!(target: INDEXER, "Block processing failed: {}", e);
+ Err(Error::BlockProcessingFailed(e))
+ }
+ }Likely invalid or redundant comment.
indexer/src/fastnear_indexer.rs (4)
24-24: Parameterise the endpoint URL instead of hardcoding it
Hardcoding the endpoint URL restricts flexibility and complicates deployment configurations. The endpoint should be configurable through environment variables or configuration files.
130-130: Implement exponential back-off on fetch failures to prevent rapid retries
Currently, the code retries fetching the latest block every second, even after a failure. This can lead to unnecessary load and potential throttling. Introduce exponential back-off to space out retries after failures.
177-184: Consider all execution success variants in is_successful_execution
The function only treats SuccessValue with an empty value as successful, ignoring other success statuses like SuccessValue with data or SuccessReceiptId. This may cause valid successful executions to be overlooked.
93-94: Handle cases where tx_hash is None to avoid missing valid data
By skipping processing when tx_hash is None, you may miss relevant data. Adjust the logic to handle cases where tx_hash is missing.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
indexer/src/main.rs (2)
Line range hint
10-22: Module organisation requires improvementThe new
fastnear_indexermodule declaration appears at the end of the module list, breaking alphabetical ordering. This inconsistency hampers code maintainability.Apply this diff to maintain alphabetical ordering:
mod block_listener; mod candidates_validator; mod configs; mod errors; +mod fastnear_indexer; mod indexer_wrapper; mod metrics; mod metrics_server; mod rmq_publisher; -mod types; -mod fastnear_indexer; +mod types;
111-113: Logging configuration needs restructuringThe logging filter is a hard-coded string that's difficult to maintain and prone to formatting errors. It should be constructed programmatically.
Consider restructuring:
- let env_filter = near_o11y::tracing_subscriber::EnvFilter::new( - "nearcore=info,publisher=info,indexer=info,candidates_validator=info,\ - metrics=info,tokio_reactor=info,near=info,stats=info,telemetry=info,\ - near-performance-metrics=info,fastnear_indexer=info", - ); + let components = vec![ + "nearcore", "publisher", "indexer", "candidates_validator", + "metrics", "tokio_reactor", "near", "stats", "telemetry", + "near-performance-metrics", "fastnear_indexer" + ]; + let env_filter = near_o11y::tracing_subscriber::EnvFilter::new( + components.iter() + .map(|c| format!("{}=info", c)) + .collect::<Vec<_>>() + .join(",") + );indexer/src/configs.rs (1)
56-61: Architectural concern: Configuration couplingThe addition of FastNear-specific configuration directly into the core RunConfigArgs creates tight coupling. Consider extracting FastNear-specific configuration into a separate struct:
+ #[derive(clap::Parser, Deserialize, Debug)] + pub struct FastNearConfig { + /// Address of fastnear block producer. + #[clap(long, default_value = "https://testnet.neardata.xyz/v0/last_block/final")] + pub address: String, + + /// Internal FastIndexer channels width. + #[clap(long, default_value = "256")] + pub channel_width: usize, + } #[derive(clap::Parser, Deserialize, Debug)] #[group(id = "config_args", conflicts_with = "config_path")] pub(crate) struct RunConfigArgs { // ... existing fields ... - pub fastnear_address: String, - pub channel_width: usize + #[clap(flatten)] + pub fastnear: FastNearConfig, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/integration-tests.yml(1 hunks)indexer/src/configs.rs(1 hunks)indexer/src/fastnear_indexer.rs(1 hunks)indexer/src/main.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-tests.yml
🧰 Additional context used
📓 Learnings (1)
indexer/src/main.rs (2)
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/src/main.rs:43-50
Timestamp: 2024-11-28T13:49:02.995Z
Learning: In `indexer/src/main.rs`, using `if cfg!(feature = "use_fastnear")` is intended and acceptable; do not suggest replacing it with conditional compilation attributes in future reviews.
Learnt from: Fly-Style
PR: Nuffle-Labs/nffl#321
File: indexer/src/main.rs:100-100
Timestamp: 2024-11-28T13:48:31.475Z
Learning: `Error::AnyhowError` requires the `anyhow!` macro as an argument to produce an `anyhow::Error`.
🔇 Additional comments (4)
indexer/src/main.rs (1)
44-50:
Potential resource leak in FastNear stream handling
The validated stream is not properly cleaned up if the system encounters an error. This could lead to resource leaks.
Implement proper cleanup:
if cfg!(feature = "use_fastnear") {
let fastnear_indexer = FastNearIndexer::new(fastnear_addr, addr_to_rollup_ids, config.channel_width);
- let validated_stream = fastnear_indexer.run();
+ let validated_stream = fastnear_indexer.run().with_cleanup(|| {
+ info!(target: INDEXER, "Cleaning up FastNear resources");
+ });
rmq_publisher.run(validated_stream);
Ok::<_, Error>(())
} else {Likely invalid or redundant comment.
indexer/src/configs.rs (1)
59-61:
Previous documentation concerns remain unaddressed
The channel_width parameter still lacks proper documentation and validation as highlighted in the previous review.
Additionally, this parameter requires integration testing to validate its impact on system performance:
#!/bin/bash
# Search for performance tests or benchmarks related to channel_width
rg -g '*.rs' -l 'channel_width.*bench|bench.*channel_width'
# Look for any existing performance metrics collection
ast-grep --pattern 'metrics.$$$channel_width$$$'indexer/src/fastnear_indexer.rs (2)
128-144:
Implement exponential backoff for error handling.
The current implementation uses a fixed 1-second delay between retries, which could overwhelm the system during persistent failures.
+ let mut backoff = 1;
loop {
match Self::fetch_latest_block(&client, fastnear_address.as_str()).await {
Ok(block) => {
let block_height = block.block.header.height;
if block_sender.send(block).await.is_err() {
error!(FASTNEAR_INDEXER, "Failed to send block to channel");
break;
}
info!(
FASTNEAR_INDEXER,
"Successfully fetched and sent latest block with id: {}", block_height
);
+ backoff = 1;
}
- Err(e) => error!(FASTNEAR_INDEXER, "Error fetching latest block: {:?}", e),
+ Err(e) => {
+ error!(FASTNEAR_INDEXER, "Error fetching latest block: {:?}", e);
+ backoff = std::cmp::min(backoff * 2, 60);
+ }
}
- tokio::time::sleep(std::time::Duration::from_secs(1)).await;
+ tokio::time::sleep(std::time::Duration::from_secs(backoff)).await;
}Likely invalid or redundant comment.
190-197:
Broaden successful execution criteria.
The current implementation only considers empty success values as successful executions, potentially missing valid successful executions with non-empty values.
fn is_successful_execution(receipt_execution_outcome: &IndexerExecutionOutcomeWithReceiptAndTxHash) -> bool {
let is_successful = matches!(
receipt_execution_outcome.execution_outcome.outcome.status,
- ExecutionStatusView::SuccessValue(ref value) if value.is_empty()
+ ExecutionStatusView::SuccessValue(_) | ExecutionStatusView::SuccessReceiptId(_)
);
trace!(FASTNEAR_INDEXER, "Execution successful: {}", is_successful);
is_successful
}Likely invalid or redundant comment.
New Features
nearcoresetup in docker-compose environment and will be shipped as separate PR (NFFL FastNear indexer: CI test #322)Improvements
Bug Fixes
This change is
Summary by CodeRabbit
Release Notes
New Features
FastNearIndexerfor enhanced interaction with the NEAR blockchain.channel_widthin command-line arguments.Improvements
reqwestdependency to version 0.12.9 with thejsonfeature.Bug Fixes
agreator-healthtoaggregator-healthin Docker configurations.Documentation