Feat/backend stellar bindings #210
Conversation
📝 WalkthroughWalkthroughRefactors backend Soroban integration to a centralized, config-driven system: adds Soroban config, robust transaction utilities (build/simulate/submit/confirm/retry), structured error classes, and updates booking/property modules to use these utilities with optional mock mode. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/blockchain/propertyListingContract.ts (1)
104-113: Avoid mutatingproperty.amenitieswhen hashing.
amenities.sort()mutates the input array and can surprise callers. Use a copied array instead.💡 Suggested fix
- amenities: property.amenities.sort(), // Sort for consistency + amenities: [...property.amenities].sort(), // Sort for consistency without mutating input
🤖 Fix all issues with AI agents
In `@apps/backend/src/blockchain/bookingContract.ts`:
- Around line 43-45: The timestamp types for startDateScVal and endDateScVal in
checkBookingAvailability are incorrectly using 'i64'; change them to 'u64' to
match the contract ABI and soroban.ts usage. Update the two
StellarSdk.nativeToScVal calls (startDateScVal and endDateScVal) to pass { type:
'u64' } and ensure the startDate/endDate values are provided as appropriate
numeric/BigInt types accepted by nativeToScVal. Leave propertyIdScVal as-is.
In `@apps/backend/src/blockchain/config.ts`:
- Around line 121-136: The config initialization currently allows empty
bookingContractId and propertyContractId which causes confusing runtime errors;
after computing bookingContractId and propertyContractId (the variables in
blockchain/config.ts) and before building the config object, add a validation
step that, when not running in mock mode, checks that both bookingContractId and
propertyContractId are non-empty and throws a clear Error (or calls
process.exit(1) with a descriptive log) naming the missing contract ID(s) so
initialization fails fast; place this check just before the config = { ... }
block so sourceKeypair and network values remain intact.
In `@apps/backend/src/blockchain/propertyListingContract.ts`:
- Around line 301-307: The function updatePropertyStatus currently returns a
hardcoded 'updated_hash'; instead, extract and return the real on‑chain hash or
re-fetch the listing after the transaction succeeds. Locate updatePropertyStatus
and use the contract result (e.g., result.returnValue or the contract's returned
fields) to obtain the actual data_hash, or call the existing getter (e.g.,
getPropertyListing(propertyId) or similar) after result.status === 'SUCCESS' to
retrieve the up‑to‑date data_hash and other fields before returning { id:
propertyId, data_hash, owner: ownerAddress, status }.
In `@apps/backend/src/blockchain/soroban.ts`:
- Around line 38-41: checkAvailability, getAccountUSDCBalance, and
verifyStellarTransaction currently assume getSorobanConfig() returns non-null
servers and will crash when USE_MOCK=true (config.servers === null); add an
early guard in each function that checks the soroban config (via
getSorobanConfig() or config.servers) and either return a sensible mock response
or throw a clear "not supported in mock mode" error; ensure the guard is placed
at the top of the functions (before any dereference of config.servers or RPC
calls) and reference the function names checkAvailability,
getAccountUSDCBalance, and verifyStellarTransaction so callers fail fast in mock
mode.
In `@apps/backend/src/blockchain/transactionUtils.ts`:
- Around line 91-94: The TransactionOptions.memo is declared but never applied
in build/send logic, so either implement memo wiring or remove it from the
interface; to fix, update the function(s) in transactionUtils.ts that construct
transactions (e.g., where TransactionOptions is consumed) to add the memo when
options?.memo is truthy — call the appropriate transaction builder method or
field (e.g., setMemo or memo property on the transaction object) so the memo is
included in the signed/serialized tx, or alternatively remove the memo property
from the TransactionOptions interface and delete the empty if (options?.memo) {
... } block to avoid silently ignoring caller input.
🧹 Nitpick comments (3)
apps/backend/src/blockchain/transactionUtils.ts (3)
1-17: Unused import:Keypair
Keypairis imported but never used in this file.Suggested fix
import { Transaction, TransactionBuilder, - Keypair, xdr, scValToNative, } from '@stellar/stellar-sdk';
127-139: Brittle 404 detection relies on specific error shapeThe check
error?.response?.status === 404assumes a particular HTTP client error structure (Axios-style). If the SDK changes its HTTP implementation or wraps errors differently, this could break.Consider also checking for common error patterns like
error.message.includes('404')or the error name as a fallback.Suggested improvement
} catch (error: any) { // 404 means transaction not yet confirmed, keep polling - if (error?.response?.status === 404) { + const is404 = + error?.response?.status === 404 || + error?.status === 404 || + (error?.message && /not found|404/i.test(error.message)); + if (is404) { await new Promise((resolve) => setTimeout(resolve, pollInterval)); continue; }
195-210: Cache sorobanMeta and clarify magic numberTwo valid concerns:
meta.switch() === 3is a magic number. While 3 correctly identifies TransactionMetaV3 (no named constant exists in the SDK v13.3.0), a local constant improves readability.
meta.v3().sorobanMeta()is called three times (lines 203, 204, 206), causing redundant XDR parsing. Cache the result instead.Suggested improvement
// Extract return value from the transaction result + const TRANSACTION_META_V3 = 3; if ( - meta.switch() === 3 && - meta.v3().sorobanMeta() && - meta.v3().sorobanMeta()?.returnValue() + meta.switch() === TRANSACTION_META_V3 ) { - const scVal = meta.v3().sorobanMeta()?.returnValue(); - if (scVal) { - returnValue = scValToNative(scVal); + const sorobanMeta = meta.v3().sorobanMeta(); + const scVal = sorobanMeta?.returnValue(); + if (sorobanMeta && scVal) { + returnValue = scValToNative(scVal); } }
| const propertyIdScVal = StellarSdk.nativeToScVal(propertyId, { type: 'string' }); | ||
| const startDateScVal = StellarSdk.nativeToScVal(startDate, { type: 'i64' }); | ||
| const endDateScVal = StellarSdk.nativeToScVal(endDate, { type: 'i64' }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the files mentioned in the review
find . -name "bookingContract.ts" -o -name "soroban.ts" | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 206
🏁 Script executed:
# Get file structure in the blockchain directory
fd . apps/backend/src/blockchain --type fRepository: Stellar-Rent/stellar-rent
Length of output: 460
🏁 Script executed:
# Look for contract definitions or ABI files
fd -e sol -e rs -e json apps/backend/src/blockchain | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 482
🏁 Script executed:
# Read bookingContract.ts with line numbers to see context around lines 43-45
cat -n apps/backend/src/blockchain/bookingContract.ts | head -70Repository: Stellar-Rent/stellar-rent
Length of output: 2689
🏁 Script executed:
# Read soroban.ts to see how timestamps are encoded there
cat -n apps/backend/src/blockchain/soroban.tsRepository: Stellar-Rent/stellar-rent
Length of output: 8622
🏁 Script executed:
# Search for contract spec or ABI files
fd -e json -e spec -e abi apps/backend | grep -i bookingRepository: Stellar-Rent/stellar-rent
Length of output: 422
🏁 Script executed:
# Check the mock file for hints about types
cat apps/backend/src/__mocks__/bookingContract.tsRepository: Stellar-Rent/stellar-rent
Length of output: 1557
🏁 Script executed:
# Search for usage of these functions to determine which is canonical
rg "checkBookingAvailability|checkAvailability" apps/backend/src --type ts -A 2 -B 2Repository: Stellar-Rent/stellar-rent
Length of output: 7492
🏁 Script executed:
# Search for contract spec or ABI files more broadly
fd --full-path -e json -e rs -e spec apps/backend | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 121
🏁 Script executed:
# Look for any contract definition files or documentation
fd --full-path "contract" apps/backend -type f | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 239
🏁 Script executed:
# Search for Soroban contract specs in the repo root
fd -e json -e rs -e sol | grep -i contract | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 1844
🏁 Script executed:
# Check the ABI file to see what types are expected for check_availability
cat apps/stellar-contracts/booking_abi.json | head -100Repository: Stellar-Rent/stellar-rent
Length of output: 2147
🏁 Script executed:
# Look for the check_availability function definition in the contract
cat apps/stellar-contracts/contracts/booking/src/lib.rs | grep -A 10 "check_availability"Repository: Stellar-Rent/stellar-rent
Length of output: 722
Fix timestamp type mismatch in checkBookingAvailability.
The contract ABI specifies check_availability expects u64 for start_date and end_date, but this function uses i64. This type mismatch will cause the contract call to fail. Update to match the contract specification and align with soroban.ts, which correctly uses u64.
Fix
- const startDateScVal = StellarSdk.nativeToScVal(startDate, { type: 'i64' });
- const endDateScVal = StellarSdk.nativeToScVal(endDate, { type: 'i64' });
+ const startDateScVal = StellarSdk.nativeToScVal(startDate, { type: 'u64' });
+ const endDateScVal = StellarSdk.nativeToScVal(endDate, { type: 'u64' });🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/bookingContract.ts` around lines 43 - 45, The
timestamp types for startDateScVal and endDateScVal in checkBookingAvailability
are incorrectly using 'i64'; change them to 'u64' to match the contract ABI and
soroban.ts usage. Update the two StellarSdk.nativeToScVal calls (startDateScVal
and endDateScVal) to pass { type: 'u64' } and ensure the startDate/endDate
values are provided as appropriate numeric/BigInt types accepted by
nativeToScVal. Leave propertyIdScVal as-is.
| // Get contract IDs (with defaults for backward compatibility) | ||
| const bookingContractId = process.env.SOROBAN_CONTRACT_ID || ''; | ||
| const propertyContractId = | ||
| process.env.PROPERTY_LISTING_CONTRACT_ID || process.env.SOROBAN_CONTRACT_ID || ''; | ||
|
|
||
| // Build configuration | ||
| config = { | ||
| rpcServer, | ||
| horizonServer, | ||
| networkPassphrase: | ||
| process.env.SOROBAN_NETWORK_PASSPHRASE || Networks.TESTNET, | ||
| sourceKeypair, | ||
| contractIds: { | ||
| booking: bookingContractId, | ||
| property: propertyContractId, | ||
| }, |
There was a problem hiding this comment.
Fail fast when contract IDs are missing.
In non-mock mode, empty IDs will surface as late, confusing contract errors. Validate them during config initialization.
🐛 Proposed fix
- const bookingContractId = process.env.SOROBAN_CONTRACT_ID || '';
- const propertyContractId =
- process.env.PROPERTY_LISTING_CONTRACT_ID || process.env.SOROBAN_CONTRACT_ID || '';
+ const bookingContractId = process.env.SOROBAN_CONTRACT_ID;
+ const propertyContractId =
+ process.env.PROPERTY_LISTING_CONTRACT_ID ?? bookingContractId;
+
+ if (!bookingContractId) {
+ throw new Error('SOROBAN_CONTRACT_ID environment variable is required');
+ }
+ if (!propertyContractId) {
+ throw new Error('PROPERTY_LISTING_CONTRACT_ID environment variable is required');
+ }📝 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.
| // Get contract IDs (with defaults for backward compatibility) | |
| const bookingContractId = process.env.SOROBAN_CONTRACT_ID || ''; | |
| const propertyContractId = | |
| process.env.PROPERTY_LISTING_CONTRACT_ID || process.env.SOROBAN_CONTRACT_ID || ''; | |
| // Build configuration | |
| config = { | |
| rpcServer, | |
| horizonServer, | |
| networkPassphrase: | |
| process.env.SOROBAN_NETWORK_PASSPHRASE || Networks.TESTNET, | |
| sourceKeypair, | |
| contractIds: { | |
| booking: bookingContractId, | |
| property: propertyContractId, | |
| }, | |
| // Get contract IDs (with validation for required environment variables) | |
| const bookingContractId = process.env.SOROBAN_CONTRACT_ID; | |
| const propertyContractId = | |
| process.env.PROPERTY_LISTING_CONTRACT_ID ?? bookingContractId; | |
| if (!bookingContractId) { | |
| throw new Error('SOROBAN_CONTRACT_ID environment variable is required'); | |
| } | |
| if (!propertyContractId) { | |
| throw new Error('PROPERTY_LISTING_CONTRACT_ID environment variable is required'); | |
| } | |
| // Build configuration | |
| config = { | |
| rpcServer, | |
| horizonServer, | |
| networkPassphrase: | |
| process.env.SOROBAN_NETWORK_PASSPHRASE || Networks.TESTNET, | |
| sourceKeypair, | |
| contractIds: { | |
| booking: bookingContractId, | |
| property: propertyContractId, | |
| }, |
🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/config.ts` around lines 121 - 136, The config
initialization currently allows empty bookingContractId and propertyContractId
which causes confusing runtime errors; after computing bookingContractId and
propertyContractId (the variables in blockchain/config.ts) and before building
the config object, add a validation step that, when not running in mock mode,
checks that both bookingContractId and propertyContractId are non-empty and
throws a clear Error (or calls process.exit(1) with a descriptive log) naming
the missing contract ID(s) so initialization fails fast; place this check just
before the config = { ... } block so sourceKeypair and network values remain
intact.
| if (result.status === 'SUCCESS') { | ||
| return { | ||
| id: propertyId, | ||
| data_hash: 'updated_hash', | ||
| owner: ownerAddress, | ||
| status, | ||
| }; |
There was a problem hiding this comment.
updatePropertyStatus returns a placeholder hash.
Returning 'updated_hash' breaks integrity checks and can overwrite correct data in callers. Return the actual on‑chain hash (from the contract return value) or re-fetch the listing after success.
🛠️ One safe approach (re-fetch after success)
- if (result.status === 'SUCCESS') {
- return {
- id: propertyId,
- data_hash: 'updated_hash',
- owner: ownerAddress,
- status,
- };
- }
+ if (result.status === 'SUCCESS') {
+ const listing = await getPropertyListing(propertyId);
+ if (listing) return listing;
+ throw new ContractError(
+ 'Status updated but updated listing could not be retrieved',
+ config.contractIds.property
+ );
+ }🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/propertyListingContract.ts` around lines 301 -
307, The function updatePropertyStatus currently returns a hardcoded
'updated_hash'; instead, extract and return the real on‑chain hash or re-fetch
the listing after the transaction succeeds. Locate updatePropertyStatus and use
the contract result (e.g., result.returnValue or the contract's returned fields)
to obtain the actual data_hash, or call the existing getter (e.g.,
getPropertyListing(propertyId) or similar) after result.status === 'SUCCESS' to
retrieve the up‑to‑date data_hash and other fields before returning { id:
propertyId, data_hash, owner: ownerAddress, status }.
| async function checkAvailability(request: AvailabilityRequest): Promise<AvailabilityResponse> { | ||
| const config = getSorobanConfig(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Mock mode will crash these functions without a guard.
When USE_MOCK=true, the config uses null servers; these functions then dereference them. Add a mock short‑circuit (or explicit “not supported in mock” error) in checkAvailability, getAccountUSDCBalance, and verifyStellarTransaction.
🛠️ Example guard for checkAvailability (apply similarly elsewhere)
const config = getSorobanConfig();
+ if (config.useMock) {
+ return { isAvailable: true, conflictingBookings: [] };
+ }📝 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.
| async function checkAvailability(request: AvailabilityRequest): Promise<AvailabilityResponse> { | |
| const config = getSorobanConfig(); | |
| try { | |
| async function checkAvailability(request: AvailabilityRequest): Promise<AvailabilityResponse> { | |
| const config = getSorobanConfig(); | |
| if (config.useMock) { | |
| return { isAvailable: true, conflictingBookings: [] }; | |
| } | |
| try { |
🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/soroban.ts` around lines 38 - 41,
checkAvailability, getAccountUSDCBalance, and verifyStellarTransaction currently
assume getSorobanConfig() returns non-null servers and will crash when
USE_MOCK=true (config.servers === null); add an early guard in each function
that checks the soroban config (via getSorobanConfig() or config.servers) and
either return a sensible mock response or throw a clear "not supported in mock
mode" error; ensure the guard is placed at the top of the functions (before any
dereference of config.servers or RPC calls) and reference the function names
checkAvailability, getAccountUSDCBalance, and verifyStellarTransaction so
callers fail fast in mock mode.
| // Add memo if provided | ||
| if (options?.memo) { | ||
| // Could add memo support here if needed | ||
| } |
There was a problem hiding this comment.
Incomplete memo implementation creates misleading API
The TransactionOptions interface exposes a memo parameter, but this code block is empty. Callers may pass a memo expecting it to be added, but it will be silently ignored.
Either implement memo support or remove the memo field from the interface to avoid confusion.
Option 1: Implement memo support
// Add memo if provided
if (options?.memo) {
- // Could add memo support here if needed
+ const { Memo } = await import('@stellar/stellar-sdk');
+ txBuilder.addMemo(Memo.text(options.memo));
}Option 2: Remove from interface
export interface TransactionOptions {
fee?: string;
timeout?: number;
- memo?: string;
}And remove lines 91-94.
📝 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.
| // Add memo if provided | |
| if (options?.memo) { | |
| // Could add memo support here if needed | |
| } | |
| // Add memo if provided | |
| if (options?.memo) { | |
| const { Memo } = await import('@stellar/stellar-sdk'); | |
| txBuilder.addMemo(Memo.text(options.memo)); | |
| } |
| // Add memo if provided | |
| if (options?.memo) { | |
| // Could add memo support here if needed | |
| } |
🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/transactionUtils.ts` around lines 91 - 94, The
TransactionOptions.memo is declared but never applied in build/send logic, so
either implement memo wiring or remove it from the interface; to fix, update the
function(s) in transactionUtils.ts that construct transactions (e.g., where
TransactionOptions is consumed) to add the memo when options?.memo is truthy —
call the appropriate transaction builder method or field (e.g., setMemo or memo
property on the transaction object) so the memo is included in the
signed/serialized tx, or alternatively remove the memo property from the
TransactionOptions interface and delete the empty if (options?.memo) { ... }
block to avoid silently ignoring caller input.
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
Thanks for the pr @KevinMB0220 ! It looks promising, but we have a few blockers: the import from @stellar/stellar-sdk/horizon is broken in v13 (use the main entry point instead), and we need the integration tests to pass on testnet without mocks (USE_MOCK=false) to meet the issue criteria. Please also standardize the dependencies by removing the duplicate stellar-sdk in favor of @stellar/stellar-sdk, ensure propertyId types are consistent (Address vs. String), and clean up the PRD comments in propertyListingContract.ts.
Ok i work in that |
|
@respp done |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/backend/src/blockchain/propertyListingContract.ts`:
- Around line 179-185: The code currently returns a hardcoded status 'Available'
in the success branch which can overwrite the real listing state; inside the
success branch of the function handling result.status === 'SUCCESS' (the block
that returns an object with id: propertyId, data_hash: dataHash, owner:
ownerAddress, status: 'Available'), either preserve the existing listing status
(e.g., use an existingListing.status variable if available) or fetch the updated
listing from the contract (call the contract's read method such as
getListing/getProperty by propertyId) and set status to that value before
returning; update the return object to use the fetched/preserved status instead
of the literal 'Available'.
In `@apps/backend/src/blockchain/soroban.ts`:
- Around line 179-202: The code incorrectly casts Horizon operation records to
Stellar SDK Operation.Payment and attempts to use an Asset object; instead treat
the item from operationsResponse.records (paymentOperation / paymentDetails) as
a Horizon operation record and use its flat properties: check
paymentDetails.asset_type === 'native' to detect XLM, compare
paymentDetails.asset_code to expectedAssetCode, and compare
paymentDetails.asset_issuer to USDC_ISSUER; remove the cast to
Operation.Payment/Asset and any calls like isNative(), and update validations to
use asset_type, asset_code, asset_issuer, and amount fields of the Horizon
record.
In `@apps/backend/tests/integration/testnet-confirmation.test.ts`:
- Around line 140-148: The test uses an invalid Stellar public key string which
fails client-side checksum validation and exercises the wrong error path; update
the fakePublicKey constant in the test (in
apps/backend/tests/integration/testnet-confirmation.test.ts inside the 'should
return 0 balance for invalid account' test that calls getAccountUSDCBalance) to
a syntactically valid Stellar testnet public key (a properly checksummed 'G...'
address that is unfunded/non-existent on testnet) so the Stellar SDK will query
the network and trigger the account-not-found branch rather than a
decode/validation error.
In `@apps/backend/tests/integration/testnet-transactions.test.ts`:
- Around line 105-106: The test constructs timestamp ScVals using nativeToScVal
with { type: 'i64' } (variables fromScVal and toScVal) which mismatches the
implementation that expects { type: 'u64' } for the check_availability contract
call; update the test to use { type: 'u64' } when creating fromScVal and toScVal
(via nativeToScVal) so the encoded types align with the contract's u64
parameters.
♻️ Duplicate comments (2)
apps/backend/src/blockchain/soroban.ts (1)
38-41: Mock mode guard still missing.The previous review flagged this: when
USE_MOCK=true,config.rpcServerandconfig.horizonServerarenull, causing crashes incheckAvailability,getAccountUSDCBalance, andverifyStellarTransaction. Add early guards to return mock data or throw explicit errors.apps/backend/src/blockchain/propertyListingContract.ts (1)
246-252: Placeholder'updated_hash'still present.This was flagged previously: returning a hardcoded
'updated_hash'breaks integrity checks. Retrieve the actual on-chain hash from the contract return value or re-fetch viagetPropertyListingafter success.
🧹 Nitpick comments (4)
apps/backend/tests/integration/testnet-soroban.test.ts (1)
96-102: Timeout unit assumption could be documented.The assertion
config.timeouts.confirmation > config.timeouts.transaction * 1000assumestransactionis in seconds andconfirmationis in milliseconds. This is correct based on config.ts defaults, but consider adding a brief comment to clarify the expected units for future maintainers.apps/backend/tests/integration/testnet-confirmation.test.ts (1)
64-64: Replaceexpect(true).toBe(false)with explicit failure.Using
expect(true).toBe(false)as a "should not reach here" check is an anti-pattern. Consider usingexpect.fail()or simply removing the try-catch wrapper and letting the test fail naturally if no error is thrown.♻️ Proposed fix
- expect(true).toBe(false); // Should not reach here + expect.fail('Expected error to be thrown');Also applies to: 78-78
apps/backend/tests/integration/testnet-transactions.test.ts (2)
5-5: Unused importsubmitAndConfirmTransaction.This function is imported but only used in the commented-out test section. Remove or comment out until the expensive tests are enabled.
164-164: Replaceexpect(true).toBe(false)with explicit failure.Same anti-pattern as in other test files.
♻️ Proposed fix
- expect(true).toBe(false); // Should not reach here + expect.fail('Expected error to be thrown');
| if (result.status === 'SUCCESS') { | ||
| return { | ||
| id: propertyId, | ||
| data_hash: dataHash, | ||
| owner: ownerAddress, | ||
| status: 'Available', | ||
| }; |
There was a problem hiding this comment.
Hardcoded status 'Available' may overwrite existing status.
When updating a property listing, the returned status is always 'Available', which may not reflect the actual property status (e.g., if it was 'Booked' or 'Maintenance'). Consider preserving the existing status or fetching the updated listing from the contract.
🛠️ Option: Fetch the updated listing to get accurate status
if (result.status === 'SUCCESS') {
- return {
- id: propertyId,
- data_hash: dataHash,
- owner: ownerAddress,
- status: 'Available',
- };
+ const listing = await getPropertyListing(propertyId);
+ if (listing) return listing;
+ // Fallback if fetch fails
+ return {
+ id: propertyId,
+ data_hash: dataHash,
+ owner: ownerAddress,
+ status: 'Available',
+ };
}📝 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.
| if (result.status === 'SUCCESS') { | |
| return { | |
| id: propertyId, | |
| data_hash: dataHash, | |
| owner: ownerAddress, | |
| status: 'Available', | |
| }; | |
| if (result.status === 'SUCCESS') { | |
| const listing = await getPropertyListing(propertyId); | |
| if (listing) return listing; | |
| // Fallback if fetch fails | |
| return { | |
| id: propertyId, | |
| data_hash: dataHash, | |
| owner: ownerAddress, | |
| status: 'Available', | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/propertyListingContract.ts` around lines 179 -
185, The code currently returns a hardcoded status 'Available' in the success
branch which can overwrite the real listing state; inside the success branch of
the function handling result.status === 'SUCCESS' (the block that returns an
object with id: propertyId, data_hash: dataHash, owner: ownerAddress, status:
'Available'), either preserve the existing listing status (e.g., use an
existingListing.status variable if available) or fetch the updated listing from
the contract (call the contract's read method such as getListing/getProperty by
propertyId) and set status to that value before returning; update the return
object to use the fetched/preserved status instead of the literal 'Available'.
| const paymentDetails = paymentOperation as unknown as Operation.Payment; | ||
|
|
||
| if (paymentDetails.destination !== expectedDestination) { | ||
| throw new Error( | ||
| `Payment destination mismatch. Expected ${expectedDestination}, got ${paymentDetails.destination}` | ||
| ); | ||
| } | ||
|
|
||
| // Check asset details directly from paymentDetails.asset | ||
| if (paymentDetails.asset.isNative()) { | ||
| throw new Error(`Native (XLM) asset used in payment. Expected ${expectedAssetCode}.`); | ||
| } | ||
|
|
||
| const paymentAsset = paymentDetails.asset as Asset; // Cast to Asset to access code and issuer | ||
| if (paymentAsset.code !== expectedAssetCode) { | ||
| throw new Error( | ||
| `Payment asset code mismatch. Expected ${expectedAssetCode}, got ${paymentAsset.code}` | ||
| ); | ||
| } | ||
| if (paymentAsset.issuer !== USDC_ISSUER) { | ||
| throw new Error( | ||
| `Payment asset issuer mismatch. Expected ${USDC_ISSUER}, got ${paymentAsset.issuer}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incorrect type cast for Horizon operation records.
operationsResponse.records returns Horizon operation records, not Stellar SDK Operation.Payment objects. Horizon records have flat properties (asset_type, asset_code, asset_issuer, amount) rather than an Asset object with methods like isNative(). This will cause runtime errors.
🐛 Proposed fix using Horizon record properties
- const paymentDetails = paymentOperation as unknown as Operation.Payment;
+ // Horizon payment operation record structure
+ const paymentDetails = paymentOperation as unknown as {
+ to: string;
+ from: string;
+ amount: string;
+ asset_type: string;
+ asset_code?: string;
+ asset_issuer?: string;
+ };
- if (paymentDetails.destination !== expectedDestination) {
+ if (paymentDetails.to !== expectedDestination) {
throw new Error(
- `Payment destination mismatch. Expected ${expectedDestination}, got ${paymentDetails.destination}`
+ `Payment destination mismatch. Expected ${expectedDestination}, got ${paymentDetails.to}`
);
}
- // Check asset details directly from paymentDetails.asset
- if (paymentDetails.asset.isNative()) {
+ if (paymentDetails.asset_type === 'native') {
throw new Error(`Native (XLM) asset used in payment. Expected ${expectedAssetCode}.`);
}
- const paymentAsset = paymentDetails.asset as Asset; // Cast to Asset to access code and issuer
- if (paymentAsset.code !== expectedAssetCode) {
+ if (paymentDetails.asset_code !== expectedAssetCode) {
throw new Error(
- `Payment asset code mismatch. Expected ${expectedAssetCode}, got ${paymentAsset.code}`
+ `Payment asset code mismatch. Expected ${expectedAssetCode}, got ${paymentDetails.asset_code}`
);
}
- if (paymentAsset.issuer !== USDC_ISSUER) {
+ if (paymentDetails.asset_issuer !== USDC_ISSUER) {
throw new Error(
- `Payment asset issuer mismatch. Expected ${USDC_ISSUER}, got ${paymentAsset.issuer}`
+ `Payment asset issuer mismatch. Expected ${USDC_ISSUER}, got ${paymentDetails.asset_issuer}`
);
}📝 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.
| const paymentDetails = paymentOperation as unknown as Operation.Payment; | |
| if (paymentDetails.destination !== expectedDestination) { | |
| throw new Error( | |
| `Payment destination mismatch. Expected ${expectedDestination}, got ${paymentDetails.destination}` | |
| ); | |
| } | |
| // Check asset details directly from paymentDetails.asset | |
| if (paymentDetails.asset.isNative()) { | |
| throw new Error(`Native (XLM) asset used in payment. Expected ${expectedAssetCode}.`); | |
| } | |
| const paymentAsset = paymentDetails.asset as Asset; // Cast to Asset to access code and issuer | |
| if (paymentAsset.code !== expectedAssetCode) { | |
| throw new Error( | |
| `Payment asset code mismatch. Expected ${expectedAssetCode}, got ${paymentAsset.code}` | |
| ); | |
| } | |
| if (paymentAsset.issuer !== USDC_ISSUER) { | |
| throw new Error( | |
| `Payment asset issuer mismatch. Expected ${USDC_ISSUER}, got ${paymentAsset.issuer}` | |
| ); | |
| } | |
| // Horizon payment operation record structure | |
| const paymentDetails = paymentOperation as unknown as { | |
| to: string; | |
| from: string; | |
| amount: string; | |
| asset_type: string; | |
| asset_code?: string; | |
| asset_issuer?: string; | |
| }; | |
| if (paymentDetails.to !== expectedDestination) { | |
| throw new Error( | |
| `Payment destination mismatch. Expected ${expectedDestination}, got ${paymentDetails.to}` | |
| ); | |
| } | |
| if (paymentDetails.asset_type === 'native') { | |
| throw new Error(`Native (XLM) asset used in payment. Expected ${expectedAssetCode}.`); | |
| } | |
| if (paymentDetails.asset_code !== expectedAssetCode) { | |
| throw new Error( | |
| `Payment asset code mismatch. Expected ${expectedAssetCode}, got ${paymentDetails.asset_code}` | |
| ); | |
| } | |
| if (paymentDetails.asset_issuer !== USDC_ISSUER) { | |
| throw new Error( | |
| `Payment asset issuer mismatch. Expected ${USDC_ISSUER}, got ${paymentDetails.asset_issuer}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/soroban.ts` around lines 179 - 202, The code
incorrectly casts Horizon operation records to Stellar SDK Operation.Payment and
attempts to use an Asset object; instead treat the item from
operationsResponse.records (paymentOperation / paymentDetails) as a Horizon
operation record and use its flat properties: check paymentDetails.asset_type
=== 'native' to detect XLM, compare paymentDetails.asset_code to
expectedAssetCode, and compare paymentDetails.asset_issuer to USDC_ISSUER;
remove the cast to Operation.Payment/Asset and any calls like isNative(), and
update validations to use asset_type, asset_code, asset_issuer, and amount
fields of the Horizon record.
| test('should return 0 balance for invalid account', async () => { | ||
| // Use a valid public key format but likely doesn't exist | ||
| const fakePublicKey = 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF'; | ||
|
|
||
| const balance = await getAccountUSDCBalance(fakePublicKey); | ||
|
|
||
| // Should return '0' for non-existent accounts | ||
| expect(balance).toBe('0'); | ||
| }, 30000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for loadAccount usage in the codebase
rg -n "loadAccount" --type ts -A 5 apps/backend/src/Repository: Stellar-Rent/stellar-rent
Length of output: 595
🏁 Script executed:
# Find and examine the test file
find apps/backend/tests -name "*testnet-confirmation.test.ts" -type fRepository: Stellar-Rent/stellar-rent
Length of output: 128
🏁 Script executed:
# Look for the getAccountUSDCBalance function implementation
rg -n "getAccountUSDCBalance" --type ts -B 2 -A 10 apps/backend/src/Repository: Stellar-Rent/stellar-rent
Length of output: 1429
🏁 Script executed:
# Check for other test patterns with public keys
rg -n "test.*balance|test.*account" --type ts -A 3 apps/backend/tests/integration/ | head -50Repository: Stellar-Rent/stellar-rent
Length of output: 2899
🏁 Script executed:
# Get the full getAccountUSDCBalance function including error handling
rg -n "async function getAccountUSDCBalance" --type ts -A 30 apps/backend/src/blockchain/soroban.tsRepository: Stellar-Rent/stellar-rent
Length of output: 1274
🏁 Script executed:
# Check the test file more completely to see all account-related tests
cat -n apps/backend/tests/integration/testnet-confirmation.test.ts | sed -n '135,155p'Repository: Stellar-Rent/stellar-rent
Length of output: 985
🏁 Script executed:
# Look for how errors are handled in retry operations
rg -n "retryOperation" --type ts -B 5 -A 15 apps/backend/src/Repository: Stellar-Rent/stellar-rent
Length of output: 26268
🌐 Web query:
Stellar SDK public key checksum validation loadAccount
💡 Result:
Short answer: Stellar SDK validates the public-key (account ID) format before calling Horizon by decoding the StrKey (base32 + version byte) and checking a CRC16-XModem checksum; if the checksum or version byte is invalid Keypair.fromPublicKey / StrKey.decode will throw "Invalid Stellar public key" and server.loadAccount will fail. [1][2][3]
What to check / do:
- Ensure you pass a valid Ed25519 account ID (starts with "G", 56 chars, no extra whitespace/newlines).
- You can pre-validate with StrKey utilities (e.g., StrKey.isValidEd25519PublicKey in JS / is_valid_ed25519_public_key in Python) before calling loadAccount.
- If you see "invalid checksum" or "invalid version byte" the StrKey encoding is corrupted or the wrong key type (seed vs pubkey) is being used. [2][1][3]
References:
- Keypair.fromPublicKey uses StrKey.decodeEd25519PublicKey (throws on invalid public key). [1]
- StrKey implementation: base32 encoding with version byte and CRC16-XModem checksum (LSB order); decode verifies checksum and version. [2]
- Python StrKey docs showing CRC16-XModem checksum used for validation. [3]
🏁 Script executed:
# Check if there are other test examples with invalid/fake public keys
rg -n "fake|invalid|GAAA" --type ts apps/backend/tests/ -B 2 -A 2Repository: Stellar-Rent/stellar-rent
Length of output: 18769
🏁 Script executed:
# Look for Stellar SDK imports and version
rg -n "@stellar/stellar-sdk" --type ts -A 1 apps/backend/src/ | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 1470
🏁 Script executed:
# Create a test script to validate the Stellar public key format
cat > /tmp/check_key.js << 'EOF'
// Check if we can validate Stellar keys
const key = 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF';
const key2 = 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHZ'; // Different last char
// Check Stellar key format (G + 55 base32 chars = 56 total)
console.log('Key length:', key.length);
console.log('Key starts with G:', key[0] === 'G');
console.log('Key:', key);
// Check if it's all As except last 3 chars
const match = key.match(/^GA{53}WHF$/);
console.log('Pattern match for GAAAA...AWHF:', !!match);
// A valid key would need correct CRC16-XModem checksum
// The fact that reviewer says it's invalid suggests checksum is wrong
EOF
node /tmp/check_key.jsRepository: Stellar-Rent/stellar-rent
Length of output: 207
🏁 Script executed:
# Look at how the wallet auth service uses Keypair.fromPublicKey with validation
rg -n "Keypair.fromPublicKey" --type ts -B 3 -A 5 apps/backend/src/Repository: Stellar-Rent/stellar-rent
Length of output: 2417
🏁 Script executed:
# Check if there are any other test patterns with valid testnet addresses
rg -n "GAAAAAAAA|public.*key|testPublicKey" --type ts apps/backend/tests/ -B 1 -A 1 | head -30Repository: Stellar-Rent/stellar-rent
Length of output: 2278
Use a properly formatted Stellar public key for the non-existent account test.
The key 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF' has an invalid checksum. The Stellar SDK validates public key checksums (via StrKey.decode) on the client side before making network requests, so loadAccount() will fail during validation rather than when querying for a non-existent account. While the error is caught and '0' is returned, the test semantically tests invalid key format instead of account non-existence. Use a valid Stellar testnet address that doesn't exist on the network to properly exercise the account-not-found path.
🤖 Prompt for AI Agents
In `@apps/backend/tests/integration/testnet-confirmation.test.ts` around lines 140
- 148, The test uses an invalid Stellar public key string which fails
client-side checksum validation and exercises the wrong error path; update the
fakePublicKey constant in the test (in
apps/backend/tests/integration/testnet-confirmation.test.ts inside the 'should
return 0 balance for invalid account' test that calls getAccountUSDCBalance) to
a syntactically valid Stellar testnet public key (a properly checksummed 'G...'
address that is unfunded/non-existent on testnet) so the Stellar SDK will query
the network and trigger the account-not-found branch rather than a
decode/validation error.
| const fromScVal = nativeToScVal(Math.floor(Date.now() / 1000), { type: 'i64' }); | ||
| const toScVal = nativeToScVal(Math.floor(Date.now() / 1000) + 86400, { type: 'i64' }); |
There was a problem hiding this comment.
Type mismatch: test uses i64 but implementation uses u64.
The test uses { type: 'i64' } for timestamp ScVals, but soroban.ts (lines 57-58) uses { type: 'u64' } for the same check_availability contract call. This inconsistency could cause simulation failures or incorrect encoding.
🐛 Fix type to match implementation
- const fromScVal = nativeToScVal(Math.floor(Date.now() / 1000), { type: 'i64' });
- const toScVal = nativeToScVal(Math.floor(Date.now() / 1000) + 86400, { type: 'i64' });
+ const fromScVal = nativeToScVal(Math.floor(Date.now() / 1000), { type: 'u64' });
+ const toScVal = nativeToScVal(Math.floor(Date.now() / 1000) + 86400, { type: 'u64' });📝 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.
| const fromScVal = nativeToScVal(Math.floor(Date.now() / 1000), { type: 'i64' }); | |
| const toScVal = nativeToScVal(Math.floor(Date.now() / 1000) + 86400, { type: 'i64' }); | |
| const fromScVal = nativeToScVal(Math.floor(Date.now() / 1000), { type: 'u64' }); | |
| const toScVal = nativeToScVal(Math.floor(Date.now() / 1000) + 86400, { type: 'u64' }); |
🤖 Prompt for AI Agents
In `@apps/backend/tests/integration/testnet-transactions.test.ts` around lines 105
- 106, The test constructs timestamp ScVals using nativeToScVal with { type:
'i64' } (variables fromScVal and toScVal) which mismatches the implementation
that expects { type: 'u64' } for the check_availability contract call; update
the test to use { type: 'u64' } when creating fromScVal and toScVal (via
nativeToScVal) so the encoded types align with the contract's u64 parameters.
2 participants
Pull Request | StellarRent
📝 Summary
This PR implements proper Soroban contract integration for the StellarRent backend, addressing GitHub Issue #195. It refactors the existing blockchain interaction layer to include centralized configuration, transaction confirmation polling, retry logic with exponential backoff, and robust error handling.
🔗 Related Issues
Closes #195 - Implement Proper Soroban Contract Integration
🔄 Changes Made
Key Finding
The backend already uses official Soroban RPC methods from
@stellar/stellar-sdkv13.3.0 - there were no custom RPC endpoints to replace. This PR focuses on improving and hardening the existing implementation.New Infrastructure (3 files)
apps/backend/src/blockchain/config.ts- Centralized ConfigurationUSE_MOCK=true)apps/backend/src/blockchain/errors.ts- Error Handling SystemNetworkError,ContractError,TransactionFailedError,TransactionTimeoutError,ConfigurationError,SimulationError,BlockchainErrorisRetryableError()to distinguish transient vs permanent failuresapps/backend/src/blockchain/transactionUtils.ts- Transaction ManagementgetFeeStats()APIRefactored Contract Files (3 files)
apps/backend/src/blockchain/bookingContract.ts'100','1000') withconfig.fees.bookingcheckBookingAvailability,createBookingOnChain,cancelBookingOnChain,updateBookingStatusOnChainapps/backend/src/blockchain/propertyListingContract.tsconfig.fees.propertycreatePropertyListing,updatePropertyListing,updatePropertyStatus,getPropertyListingapps/backend/src/blockchain/soroban.tscheckAvailability,getAccountUSDCBalance,verifyStellarTransactionConfiguration Updates
apps/backend/env.example- New Environment Variables🖼️ Current Output
Before vs After Comparison
Code Example - Before
Code Example - After
🧪 Testing
All changes are fully backwards compatible:
USE_MOCK=truecontinues to work for testing without blockchain✅ Testing Checklist
Transaction Confirmation Timeout: The new polling mechanism waits up to 60 seconds for confirmation. This increases response time but ensures reliability.
SOROBAN_CONFIRMATION_TIMEOUTenvironment variableRetry Logic: Automatic retries could delay error reporting if all attempts fail.
SOROBAN_MAX_RETRIES=3by default)Dynamic Fees: Querying network fee stats adds slight latency.
Environment Variables: New required variables must be set.
🚀 Next Steps & Improvements
This change lays a solid foundation for production-ready blockchain integration. Future enhancements could include:
Immediate Priorities
eventListener.tsto use transaction verification utilitiesFuture Optimizations
Performance Enhancements
💬 Comments
Architecture Decisions
Singleton Configuration: Chose singleton pattern to ensure single initialization and avoid redundant server connections. Thread-safe and easily testable.
Exponential Backoff: Default retry delay starts at 1000ms and doubles with each attempt, plus random jitter to prevent thundering herd.
Confirmation Polling: Polls Horizon every 2 seconds for up to 60 seconds. This ensures transactions are truly confirmed on-chain before returning success.
Error Classification: Separates network errors (retryable) from contract errors (permanent) to avoid wasting retries on non-transient failures.
Acceptance Criteria Status
Migration Guide
For deploying this update:
.envfile with new variables fromenv.exampleUSE_MOCK=falseto verify testnet connectivityTotal Changes: 3 new files, 4 modified files, ~1200 lines of robust infrastructure added
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.