feat: staking validators discovery#357
Conversation
|
This PR targeted I retargeted it to |
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a new ChangesStaking validators command
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/commands/staking.test.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelative import instead of
@/*alias.New import uses
../../src/commands/staking/validatorsrather than the configured@/*alias, per the repo's coding guidelines. This matches the pre-existing style of every other import in this file, so it's a minor, low-priority inconsistency rather than a new pattern.As per coding guidelines, "Use
@/*path alias to reference./src/*and@@/tests/*path alias to reference./tests/*in imports."Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands/staking.test.ts` at line 15, The new import in staking.test.ts uses a relative path instead of the repository’s path aliases. Update the import for ValidatorsAction (and the other affected import in this file) to use the configured `@/`* alias for src references, matching the existing import style and the repo’s import guidelines.Source: Coding guidelines
tests/commands/stakingValidators.test.ts (2)
91-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of chain-only and explorer-enriched paths; consider adding a failure-fallback test.
Both happy-path cases (no explorer, and explorer with sorting) are well covered and align with the
execute()flow shown in the upstream snippet. However, the PR objective emphasizes "graceful fallback to chain-only data" when explorer enrichment is attempted but fails/unreachable — that specific fallback path isn't tested here (only the "noexplorerUrlprovided" case is).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands/stakingValidators.test.ts` around lines 91 - 150, Add a test for the failure fallback path in stakingValidators.execute(), where explorer enrichment is attempted but the explorer request fails or is unreachable, and verify it still emits chain-only JSON instead of throwing. Reuse the existing setupAction() and JSON output assertions, but make fetchMock return a failed response for the explorer endpoint(s) and assert the output matches the chain-only shape with explorer.enabled false/null and validator performance/delegator fields left unset or null.
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
@/*alias for src import.New test file imports
ValidatorsActionvia a relative path (../../src/commands/staking/validators) instead of the@/*alias mandated for.tsfiles.As per coding guidelines, "Use
@/*path alias to reference./src/*and@@/tests/*path alias to reference./tests/*in imports."♻️ Proposed fix
-import {ValidatorsAction} from "../../src/commands/staking/validators"; +import {ValidatorsAction} from "`@/commands/staking/validators`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands/stakingValidators.test.ts` around lines 1 - 2, The test file is importing ValidatorsAction with a relative src path instead of the required alias. Update the import in stakingValidators.test.ts to use the `@/`* alias for the src/commands/staking/validators module, keeping the rest of the test setup unchanged and matching the project’s import conventions for .ts files.Source: Coding guidelines
src/commands/staking/validators.ts (1)
345-364: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGuard the pagination loop against non-advancing pages.
The loop only terminates on
validators.size < totalorpage <= 50. If the explorer reports atotallarger than what its pages actually return (or returns empty/duplicate pages),validators.sizestops growing whilepagekeeps incrementing, issuing up to ~49 extra HTTP round-trips before the cap stops it. Break early when a page yields no new entries.♻️ Proposed fix
total = typeof response.total === "number" ? response.total : response.validators.length; + const before = validators.size; for (const item of response.validators as ExplorerValidatorSummary[]) { const address = item.validator_address || item.validatorAddress; if (!address) continue; validators.set(address.toLowerCase(), this.toExplorerPerformance(item)); } + if (response.validators.length === 0 || validators.size === before) break; page += 1; } while (validators.size < total && page <= 50);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/staking/validators.ts` around lines 345 - 364, The pagination loop in the validators fetch logic can keep advancing even when a page adds no new validators, causing unnecessary requests; update the do/while in the validators API flow to stop early when a fetched page yields zero new entries. Use the existing validators map and the explorer parsing block in the fetch routine that calls this.fetchJson and toExplorerPerformance, and add a non-advancing-page check before incrementing page so the loop exits when results are empty or fully duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/staking/validators.ts`:
- Line 1: The import in validators.ts uses a relative cross-directory path to
BaseAction and should be switched to the `@/`* alias. Update the resolveNetwork
import to reference the same BaseAction symbol through the alias form used
elsewhere in src instead of ../../lib/actions/BaseAction, keeping the import
location and usage unchanged.
---
Nitpick comments:
In `@src/commands/staking/validators.ts`:
- Around line 345-364: The pagination loop in the validators fetch logic can
keep advancing even when a page adds no new validators, causing unnecessary
requests; update the do/while in the validators API flow to stop early when a
fetched page yields zero new entries. Use the existing validators map and the
explorer parsing block in the fetch routine that calls this.fetchJson and
toExplorerPerformance, and add a non-advancing-page check before incrementing
page so the loop exits when results are empty or fully duplicated.
In `@tests/commands/staking.test.ts`:
- Line 15: The new import in staking.test.ts uses a relative path instead of the
repository’s path aliases. Update the import for ValidatorsAction (and the other
affected import in this file) to use the configured `@/`* alias for src
references, matching the existing import style and the repo’s import guidelines.
In `@tests/commands/stakingValidators.test.ts`:
- Around line 91-150: Add a test for the failure fallback path in
stakingValidators.execute(), where explorer enrichment is attempted but the
explorer request fails or is unreachable, and verify it still emits chain-only
JSON instead of throwing. Reuse the existing setupAction() and JSON output
assertions, but make fetchMock return a failed response for the explorer
endpoint(s) and assert the output matches the chain-only shape with
explorer.enabled false/null and validator performance/delegator fields left
unset or null.
- Around line 1-2: The test file is importing ValidatorsAction with a relative
src path instead of the required alias. Update the import in
stakingValidators.test.ts to use the `@/`* alias for the
src/commands/staking/validators module, keeping the rest of the test setup
unchanged and matching the project’s import conventions for .ts files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46347f28-dae6-4448-8e98-7adb1ba8b799
📒 Files selected for processing (4)
src/commands/staking/index.tssrc/commands/staking/validators.tstests/commands/staking.test.tstests/commands/stakingValidators.test.ts
| @@ -0,0 +1,608 @@ | |||
| import {resolveNetwork} from "../../lib/actions/BaseAction"; | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the @/* path alias for the cross-directory src/lib import.
../../lib/actions/BaseAction resolves to src/lib/actions/BaseAction and should be referenced via the alias.
♻️ Proposed fix
-import {resolveNetwork} from "../../lib/actions/BaseAction";
+import {resolveNetwork} from "`@/lib/actions/BaseAction`";As per coding guidelines: "Use @/* path alias to reference ./src/* ... in imports".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {resolveNetwork} from "../../lib/actions/BaseAction"; | |
| import {resolveNetwork} from "`@/lib/actions/BaseAction`"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/staking/validators.ts` at line 1, The import in validators.ts
uses a relative cross-directory path to BaseAction and should be switched to the
`@/`* alias. Update the resolveNetwork import to reference the same BaseAction
symbol through the alias form used elsewhere in src instead of
../../lib/actions/BaseAction, keeping the import location and usage unchanged.
Source: Coding guidelines
1 participant
Tooling-wave:
genlayer staking validators— lists the active set with stake/status/delegator-count for CLI-only delegator onboarding (the missing piece of the full CLI delegation lifecycle). Optional --explorer-url enrichment with graceful chain-only fallback. --json for agents. Mock-based command tests.Summary by CodeRabbit
New Features
staking validatorscommand with table or JSON output.Bug Fixes
Tests