Skip to content

sync_info key change#58

Merged
devalpatel67 merged 1 commit intomainfrom
fix/node-check-issue
Oct 28, 2025
Merged

sync_info key change#58
devalpatel67 merged 1 commit intomainfrom
fix/node-check-issue

Conversation

@devalpatel67
Copy link
Copy Markdown
Collaborator

@devalpatel67 devalpatel67 commented Oct 28, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Updated Akash node readiness checks to correctly read synchronization status from the appropriate data fields, ensuring accurate node readiness determination.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 28, 2025

Walkthrough

The _check_akash_node_readiness function in the provider service was updated to read node synchronization status from lowercase sync_info keys instead of uppercase SyncInfo keys. Two field lookups were corrected: latest_block_height and catching_up now access the updated response structure.

Changes

Cohort / File(s) Summary
Node readiness sync status field update
application/service/provider_service.py
Updated _check_akash_node_readiness to read node height and catch-up status from sync_info key instead of SyncInfo for compatibility with current node status response format

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Straightforward key name correction affecting only one function
  • Consistent pattern applied uniformly across two field accesses
  • No logic changes or control-flow modifications

Poem

🐰 A sync_info hop, capitalization drop,
Keys now match the node's reply,
Readiness checks aligned with the sky,
No more "SyncInfo" to mystify! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "sync_info key change" directly corresponds to the main change in the changeset: updating the field key lookup in the _check_akash_node_readiness function from "SyncInfo" to "sync_info". The title is concise, specific, and clearly communicates the primary modification without vague terminology or unnecessary details. A developer reviewing the commit history would understand that this PR involves a key name update for node synchronization status checks.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/node-check-issue

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e41092 and 5dffa96.

📒 Files selected for processing (1)
  • application/service/provider_service.py (1 hunks)

Comment on lines +423 to +429
node_height = int(node_status["sync_info"]["latest_block_height"])
network_height = int(
network_status["result"]["sync_info"]["latest_block_height"]
)

# Check if node is catching up
if node_status["SyncInfo"]["catching_up"]:
if node_status["sync_info"]["catching_up"]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Good fix for API response key consistency.

The change from uppercase SyncInfo to lowercase sync_info aligns with the network status check at line 425, which already uses lowercase sync_info. This improves internal consistency across the function.

Please verify that the Akash node's status API response actually uses lowercase sync_info keys:


🏁 Script executed:

#!/bin/bash
# Verify the actual key structure returned by 'akash status' command
# This checks the Akash CLI documentation for the correct response format

echo "Checking Akash documentation for status command response format..."

Length of output: 66


🌐 Web query:

What is the JSON response structure of the 'akash status' command, specifically the casing of the sync_info field?

💡 Result:

The akash CLI’s printed JSON uses CamelCase "SyncInfo" (capital S and I). [1][2]

Note: programmatic RPC responses / SDK structs often use snake_case "sync_info" (e.g., Go/json struct tags or rpc JSON keys in some libraries), so if you access status via SDK/rpc you may see "sync_info" instead. [3][4]

Sources:

  • Akash docs example outputs showing "SyncInfo". [1][2]
  • Go/Cosmos-style structs and RPC/SDK examples showing "sync_info" in programmatic responses. [3][4]

I need to verify how node_status is actually obtained in this function. The web search reveals this is critical: the akash CLI uses CamelCase SyncInfo, but programmatic RPC/SDK responses use snake_case sync_info. The correctness of this change depends entirely on the data source.

Let me inspect the function to see how the status is retrieved:

I need to inspect the actual implementation of _check_akash_node_readiness to determine how node_status is retrieved. Let me search the codebase directly:

# Find and examine the _check_akash_node_readiness function
rg -n -A 30 "_check_akash_node_readiness" --type=py -B 2

This will show me how the status data is actually obtained and help confirm whether the key should be lowercase or uppercase.


</thinking>

---

🏁 Script executed:

```shell
#!/bin/bash
# Search for the _check_akash_node_readiness function to see how node_status is obtained
rg -n -B 5 -A 50 "_check_akash_node_readiness" application/service/provider_service.py | head -100

Length of output: 3071



---

🏁 Script executed:

```shell
#!/bin/bash
# Search for more of the function around the changed lines (423-429)
rg -n -A 150 "_check_akash_node_readiness" application/service/provider_service.py | sed -n '390,550p'

Length of output: 0



---

🏁 Script executed:

```shell
#!/bin/bash
# Extract lines 390-450 to see the context around the changes
sed -n '390,450p' application/service/provider_service.py

Length of output: 3020


Critical issue: Changes will break node status access.

The code executes the akash status CLI command (line 407) and parses its JSON output. According to the web search results, the akash CLI outputs CamelCase keys like SyncInfo, not lowercase sync_info. The changes at lines 423 and 429 convert these to lowercase, which will cause KeyError at runtime.

The network status fetch (lines 415-419) correctly uses RPC, which does use snake_case sync_info. The node status and network status use different data sources with different key casings:

  • Node status (from CLI): requires ["SyncInfo"] (CamelCase)
  • Network status (from RPC): requires ["sync_info"] (snake_case)

The original review comment incorrectly approved this change. Revert lines 423 and 429 to use the uppercase SyncInfo key.

🤖 Prompt for AI Agents
In application/service/provider_service.py around lines 423 to 429, the node
status JSON parsed from the akash CLI uses CamelCase keys (e.g., "SyncInfo")
whereas the network status from RPC uses snake_case; restore access to
node_status["SyncInfo"] (and its nested keys) instead of
node_status["sync_info"] so the CLI-parsed structure is accessed correctly while
leaving network_status["sync_info"] unchanged.

@devalpatel67 devalpatel67 merged commit af2d003 into main Oct 28, 2025
1 check passed
@coderabbitai coderabbitai bot mentioned this pull request Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants