fix(contract): correctly handle Soroban void (scvVoid) return values in contract.js#264
fix(contract): correctly handle Soroban void (scvVoid) return values in contract.js#264jabir-dev788 wants to merge 1 commit into
Conversation
…l to handle scvVoid correctly
📝 WalkthroughWalkthroughAdds ChangesTyped Soroban Return Value Decoding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/lib/contract.js (1)
581-599: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
ReturnValueParseErrorinstead of defaulting.These wrappers now call
parseRetval, but their catch blocks still convert parse/type failures intonull,0,-1,false, orSERVICE_READ_FAILED. That hides the ABI mismatch the new helper is intended to surface.Proposed pattern
} catch (err) { logger.error({ err, id }, 'getService failed'); + if (err instanceof ReturnValueParseError) throw err; return null; }Apply the same pattern to the other fallback wrappers; in
deactivateServiceOnChain, rethrowReturnValueParseErrorbefore wrapping read failures.Also applies to: 606-611, 759-768, 1020-1025, 1059-1064, 1075-1080, 1088-1093, 1151-1156, 1168-1173
🤖 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 `@backend/src/lib/contract.js` around lines 581 - 599, The fallback wrappers are swallowing parse/type failures from parseRetval by returning defaults like null, 0, -1, false, or SERVICE_READ_FAILED. Update getService and the other referenced wrappers to detect ReturnValueParseError in their catch blocks and rethrow it before applying any fallback, while still logging and handling genuine read failures; apply the same pattern in deactivateServiceOnChain and the other affected helper methods.
🤖 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 `@backend/src/lib/contract.js`:
- Around line 39-45: The list-return contract entries are still validated as
generic structs, which lets object-shaped responses pass and then get converted
into empty arrays by the wrappers. Update the validation in contract.js for the
list methods, especially list_services_page, list_agents, and list_agents_page,
so they are treated as array returns instead of structs. Adjust the
corresponding wrapper/validator logic used by those methods so malformed
non-array responses fail validation rather than being silently swallowed.
In `@backend/src/lib/contract.test.js`:
- Around line 929-931: The parseRetval() coverage in contract.test.js only
checks the happy path for scvVoid() with expectedType set to void; add a
negative test around parseRetval() that passes a non-void ScVal while
expectedType is 'void' and assert it throws instead of returning null. Use the
existing contractLib.parseRetval and sdkPkg.xdr.ScVal helpers so the test
targets the same coercion path, and keep it focused on the void-return mismatch
case.
---
Outside diff comments:
In `@backend/src/lib/contract.js`:
- Around line 581-599: The fallback wrappers are swallowing parse/type failures
from parseRetval by returning defaults like null, 0, -1, false, or
SERVICE_READ_FAILED. Update getService and the other referenced wrappers to
detect ReturnValueParseError in their catch blocks and rethrow it before
applying any fallback, while still logging and handling genuine read failures;
apply the same pattern in deactivateServiceOnChain and the other affected helper
methods.
🪄 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 Plus
Run ID: 1242b3d0-56a4-4e3f-a81b-3ba7df511fd9
📒 Files selected for processing (3)
backend/src/lib/contract.jsbackend/src/lib/contract.test.jsbackend/vitest.config.js
| list_services_page: 'struct', | ||
| get_service: 'struct', | ||
| get_service_count: 'u64', | ||
| // Agents contract (read) | ||
| list_agents: 'struct', | ||
| list_agents_page: 'struct', | ||
| get_agent: 'struct', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate the referenced symbols/lines.
git ls-files backend/src/lib/contract.js backend/src | sed -n '1,120p'
printf '\n--- outline contract.js ---\n'
ast-grep outline backend/src/lib/contract.js --view expanded || true
printf '\n--- targeted search ---\n'
rg -n "list_services_page|list_agents_page|list_agents|get_service|get_agent|expectedType|ReturnValueParseError|Array.isArray|struct" backend/src/lib/contract.js backend/src -g '*.js' -g '*.ts' || trueRepository: Stellar-Ecosystem/lodestar
Length of output: 17597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant slices of backend/src/lib/contract.js around the reported lines
# and the parser/validator logic around expectedType handling.
sed -n '1,140p' backend/src/lib/contract.js
printf '\n--- later section ---\n'
sed -n '520,620p' backend/src/lib/contract.js
printf '\n--- even later section ---\n'
sed -n '960,1025p' backend/src/lib/contract.jsRepository: Stellar-Ecosystem/lodestar
Length of output: 10139
Treat list returns as arrays, not generic structs. list_services_page, list_agents, and list_agents_page still use struct validation, so any object-shaped return can slip through and then get collapsed to [] by the wrappers. Add array-specific validation for these list methods so malformed responses fail instead of being silently swallowed.
🤖 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 `@backend/src/lib/contract.js` around lines 39 - 45, The list-return contract
entries are still validated as generic structs, which lets object-shaped
responses pass and then get converted into empty arrays by the wrappers. Update
the validation in contract.js for the list methods, especially
list_services_page, list_agents, and list_agents_page, so they are treated as
array returns instead of structs. Adjust the corresponding wrapper/validator
logic used by those methods so malformed non-array responses fail validation
rather than being silently swallowed.
| it('returns null for scvVoid() when expectedType is void', () => { | ||
| const scvVoid = sdkPkg.xdr.ScVal.scvVoid(); | ||
| expect(contractLib.parseRetval(scvVoid, 'void')).toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a negative test for unexpected non-void void returns.
This suite still misses the case that exposes the current silent-coercion bug: parseRetval() returns null whenever expectedType === 'void', even if the contract actually returns a value. That contradicts the PR goal of throwing on type mismatches, so a non-void retval for mutating methods would be silently accepted.
Suggested test
it('returns null for scvVoid() when expectedType is void', () => {
const scvVoid = sdkPkg.xdr.ScVal.scvVoid();
expect(contractLib.parseRetval(scvVoid, 'void')).toBeNull();
});
+
+ it('throws ReturnValueParseError when a non-void retval is returned for expectedType void', () => {
+ const scvU64 = sdkPkg.nativeToScVal(5n, { type: 'u64' });
+ expect(() => contractLib.parseRetval(scvU64, 'void')).toThrow(
+ expect.objectContaining({ name: 'ReturnValueParseError', code: 'RETURN_VALUE_PARSE_FAILED' })
+ );
+ });📝 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.
| it('returns null for scvVoid() when expectedType is void', () => { | |
| const scvVoid = sdkPkg.xdr.ScVal.scvVoid(); | |
| expect(contractLib.parseRetval(scvVoid, 'void')).toBeNull(); | |
| it('returns null for scvVoid() when expectedType is void', () => { | |
| const scvVoid = sdkPkg.xdr.ScVal.scvVoid(); | |
| expect(contractLib.parseRetval(scvVoid, 'void')).toBeNull(); | |
| }); | |
| it('throws ReturnValueParseError when a non-void retval is returned for expectedType void', () => { | |
| const scvU64 = sdkPkg.nativeToScVal(5n, { type: 'u64' }); | |
| expect(() => contractLib.parseRetval(scvU64, 'void')).toThrow( | |
| expect.objectContaining({ name: 'ReturnValueParseError', code: 'RETURN_VALUE_PARSE_FAILED' }) | |
| ); | |
| }); |
🤖 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 `@backend/src/lib/contract.test.js` around lines 929 - 931, The parseRetval()
coverage in contract.test.js only checks the happy path for scvVoid() with
expectedType set to void; add a negative test around parseRetval() that passes a
non-void ScVal while expectedType is 'void' and assert it throws instead of
returning null. Use the existing contractLib.parseRetval and sdkPkg.xdr.ScVal
helpers so the test targets the same coercion path, and keep it focused on the
void-return mismatch case.
1 participant
Problem
simulateReadandsimulateAndSubmitinbackend/src/lib/contract.jstreat ScVal return values with a truthy check (
if (!retval) return null),which fails to catch
ScVal.scvVoid()— the encoding for Rust's unit type(). SincescvVoid()is a truthy object, the guard never triggers,scValToNative()returnsundefined, and any caller wrapping the resultin
Number(...)(e.g. ID-returning routes) silently producesNaN.NaNserializes tonullin JSON, so callers see a successful-lookingnullresponse with no error or log trail.Fix
parseRetval(retval, expectedType)to explicitly distinguishnull/undefined, void, and typed return values, throwing
ReturnValueParseErroron type mismatch instead of silently coercing.CONTRACT_RETURN_TYPESmap documenting the expected return typeper contract function (also serves as runtime validation).
simulateRead/simulateAndSubmitand all call sites to usethe new helper with explicit expected types.
parseRetval(void, u64, bool, null, undefined,type mismatch) and updated affected route/service tests.
Scope
Limited to
backend/src/lib/contract.jsand its direct callers/tests.No contract (Rust) changes, no frontend changes, no unrelated refactors.
Testing
parseRetvalpassSummary by CodeRabbit
Bug Fixes
Tests