fix: ci issues#288
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates escrow release and deposit call shapes, refreshes related escrow and permission snapshots, and adjusts backend test and runtime setup for notifications, orchestrator, wallet, payments, gateway, CI, and test runners. ChangesEscrow Contract Release and Snapshot Updates
Permissions Contract Event Snapshot Updates
Backend Test and Infrastructure Fixes
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (3)
tests/unit/src/rateLimit.test.js (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider saving/restoring
MOCK_REDISfor consistency with thequeue-job-statustest pattern.The
beforehook setsMOCK_REDISbut theafterhook never cleans it up, unlike the careful save/restore pattern applied toNODE_ENVinqueue-job-status.test.jsin this same PR. Whilenode --testisolates files into separate processes, applying the same cleanup pattern here would keep the test infrastructure fixes consistent and future-proof.♻️ Suggested cleanup for consistency
describe("Gateway Rate Limiting System", () => { + let originalMockRedis; before(async () => { + originalMockRedis = process.env.MOCK_REDIS; process.env.MOCK_REDIS = "true"; // Ensure mock Redis connection is initialized safely from compiled code getRedisClient(); }); after(async () => { + if (originalMockRedis === undefined) { + delete process.env.MOCK_REDIS; + } else { + process.env.MOCK_REDIS = originalMockRedis; + } // Gracefully clean up our Redis connections await disconnectRedis(); });🤖 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/unit/src/rateLimit.test.js` at line 7, The test setup in rateLimit.test.js sets MOCK_REDIS in the `before` hook but never restores it, unlike the save/restore pattern used in the `queue-job-status` test. Update the test lifecycle around the existing `before`/`after` hooks in `rateLimit.test.js` to preserve any prior `MOCK_REDIS` value and restore or delete it afterward, using the same cleanup approach as the `queue-job-status.test.js` pattern for consistency and future-proofing.tests/unit/src/gateway-auth-handlers.test.js (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
generateTokenfrom the source file instead ofdist/.apps/backend/gateway/dist/src/auth/authService.jsisn’t tracked, so this test depends on a prior build;apps/backend/gateway/src/auth/authService.tsalready exportsgenerateToken.🤖 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/unit/src/gateway-auth-handlers.test.js` at line 11, The test is importing generateToken from the built dist artifact, which makes it depend on a prior build. Update the import in gateway-auth-handlers.test.js to reference the source authService module instead of dist, using the generateToken export from apps/backend/gateway/src/auth/authService.ts so the test runs directly from source.contracts/escrow/src/test.rs (1)
192-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicated token setup into a helper.
The same 5-line token setup block (register Stellar asset, create client, mint, add_token) is repeated in
test_deposit_with_metadata_success,test_deposit_without_metadata, andtest_deposit_with_partial_metadata. Extracting a helper would reduce duplication and ensure consistent setup.♻️ Suggested helper extraction
fn setup_token( env: &Env, buyer: &Address, admin: &Address, client: &EscrowContractClient, ) -> Address { let token_admin = Address::generate(env); let token = env.register_stellar_asset_contract_v2(token_admin).address(); let token_admin_client = soroban_sdk::token::StellarAssetClient::new(env, &token); token_admin_client.mint(buyer, &10000i128); client.add_token(admin, &token); token }Then each test replaces the 5-line block with:
let token = setup_token(&env, &buyer, &admin, &client);Also applies to: 227-230, 260-263
🤖 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 `@contracts/escrow/src/test.rs` around lines 192 - 195, The token setup logic is duplicated across the escrow tests, so extract it into a shared helper to keep the setup consistent and reduce repetition. Move the repeated register/mint/add_token sequence into a helper like setup_token in test.rs, using the same symbols Env, Address, StellarAssetClient, and EscrowContractClient, then replace each repeated block in test_deposit_with_metadata_success, test_deposit_without_metadata, and test_deposit_with_partial_metadata with a single helper call.
🤖 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 `@apps/backend/wallet/src/queue/txQueue.ts`:
- Around line 199-215: The lock retry loop in txQueue’s sequence reservation is
too short relative to the lock holder’s `lockTimeout`, and its fixed
`retryDelay` causes synchronized retries. Update the retry logic around the
`redis.set` lock acquisition so the total wait budget matches the expected lock
hold time (or derives from `lockTimeout`), add jitter/backoff to the delay, and
avoid sleeping after the last failed attempt. Use the existing lock-acquire
block in `txQueue` as the place to tune the `maxRetries`, `retryDelay`, and
failure path.
In `@contracts/escrow/test_snapshots/integration_tests/test_escrow_events.1.json`:
- Around line 707-974: The escrow snapshot is stale against the current
EscrowRecord schema: these contract_data entries still serialize unlock_time and
status-only payloads, but the persisted shape now includes timeout_ledger,
released_amount, created_at, escrow_id, and order_id. Regenerate the affected
snapshot data in test_escrow_events.1.json using the current escrow event/test
output so the EscrowRecord fixtures match the updated schema and symbols like
contract_data, Escrow, status, and unlock_time are refreshed consistently.
In
`@contracts/escrow/test_snapshots/integration_tests/test_remove_co_admin.1.json`:
- Around line 459-517: The EscrowRecord snapshot in this test fixture is missing
the new persisted released_amount field, so update the test snapshot data to
include released_amount: 0 alongside the existing EscrowRecord keys. Use the
EscrowRecord snapshot structure in test_remove_co_admin.1.json as the target and
keep it consistent with the other escrow snapshots that already include
released_amount.
---
Nitpick comments:
In `@contracts/escrow/src/test.rs`:
- Around line 192-195: The token setup logic is duplicated across the escrow
tests, so extract it into a shared helper to keep the setup consistent and
reduce repetition. Move the repeated register/mint/add_token sequence into a
helper like setup_token in test.rs, using the same symbols Env, Address,
StellarAssetClient, and EscrowContractClient, then replace each repeated block
in test_deposit_with_metadata_success, test_deposit_without_metadata, and
test_deposit_with_partial_metadata with a single helper call.
In `@tests/unit/src/gateway-auth-handlers.test.js`:
- Line 11: The test is importing generateToken from the built dist artifact,
which makes it depend on a prior build. Update the import in
gateway-auth-handlers.test.js to reference the source authService module instead
of dist, using the generateToken export from
apps/backend/gateway/src/auth/authService.ts so the test runs directly from
source.
In `@tests/unit/src/rateLimit.test.js`:
- Line 7: The test setup in rateLimit.test.js sets MOCK_REDIS in the `before`
hook but never restores it, unlike the save/restore pattern used in the
`queue-job-status` test. Update the test lifecycle around the existing
`before`/`after` hooks in `rateLimit.test.js` to preserve any prior `MOCK_REDIS`
value and restore or delete it afterward, using the same cleanup approach as the
`queue-job-status.test.js` pattern for consistency and future-proofing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56290e4b-2e21-4624-bb5b-908add3c8234
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (84)
apps/backend/gateway/src/auth/authAuditEvent.tsapps/backend/notifications/email/config.test.tsapps/backend/notifications/email/config.tsapps/backend/notifications/email/dlq.test.tsapps/backend/notifications/email/dlq.tsapps/backend/notifications/email/errorClassifier.tsapps/backend/notifications/email/index.test.tsapps/backend/notifications/email/types.tsapps/backend/notifications/src/integration.test.tsapps/backend/orchestrator/package.jsonapps/backend/orchestrator/src/index.tsapps/backend/orchestrator/workflows/checkout/index.tsapps/backend/payments/src/validation.tsapps/backend/wallet/src/queue/txQueue.tscontracts/escrow/src/integration_tests.rscontracts/escrow/src/lib.rscontracts/escrow/src/test.rscontracts/escrow/test_snapshots/integration_tests/test_accept_admin_wrong_address.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_cancel_admin_transfer.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_co_admin_accepts_primary_admin.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_co_admin_dispute_resolution.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_co_admin_restrictions.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_deposit_with_whitelisted_token_succeeds.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_dispute_blocks_release_and_refund.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_dispute_resolution_to_buyer.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_dispute_resolution_to_seller.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_double_refund_prevention.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_double_release_prevention.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_escrow_events.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_full_purchase_lifecycle.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_full_refund_lifecycle.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_full_release_via_release_still_works.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_escrow_returns_full_record.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_receipt_disputed_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_receipt_funded_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_receipt_refunded_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_receipt_released_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_old_admin_loses_privileges.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_partial_release_50_percent_stays_active.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_partial_release_exceeds_remaining_balance.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_partial_release_remaining_50_percent_released.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_partial_release_zero_amount.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_after_partial_release_refunds_unreleased_only.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_before_timeout_fails.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_admin_always_eligible.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_already_refunded.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_already_released.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_buyer_after_timeout.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_buyer_before_timeout.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_disputed.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_seller_always_eligible.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_eligibility_unauthorized_stranger.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_refund_on_released_escrow_fails.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_eligibility_disputed_blocks_release.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_eligibility_funded_before_timeout.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_eligibility_terminal_refund_blocks_release.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_eligibility_terminal_release_blocks_release.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_eligibility_timeout_blocks_release.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_on_refunded_escrow_fails.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_with_wrong_recipient_fails.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_release_wrong_caller.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_remove_co_admin.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_timeout_auto_refund.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_two_step_admin_transfer.1.jsoncontracts/escrow/test_snapshots/test/test/test_deposit_with_metadata_success.1.jsoncontracts/escrow/test_snapshots/test/test/test_deposit_with_partial_metadata.1.jsoncontracts/escrow/test_snapshots/test/test/test_deposit_without_metadata.1.jsoncontracts/escrow/test_snapshots/test/test/test_get_escrow_metadata_not_found.1.jsoncontracts/permissions/test_snapshots/test/test/test_allowance_increased_event_emitted.1.jsoncontracts/permissions/test_snapshots/test/test/test_allowance_increased_event_not_emitted_on_decrease.1.jsoncontracts/permissions/test_snapshots/test/test/test_allowance_increased_event_not_emitted_on_unchanged_limit.1.jsoncontracts/permissions/test_snapshots/test/test/test_allowance_increased_not_found.1.jsoncontracts/permissions/test_snapshots/test/test/test_merchant_list_event.1.jsoncontracts/permissions/test_snapshots/test/test/test_merchant_list_event_not_emitted.1.jsoncontracts/permissions/test_snapshots/test/test/test_non_self_delegation_succeeds.1.jsoncontracts/permissions/test_snapshots/test/test/test_self_delegation_allowed_when_config_enabled.1.jsoncontracts/permissions/test_snapshots/test/test/test_unpause_grants_allows_new_grants.1.jsoncontracts/tests/cross_contract.rscontracts/tests/test_snapshots/test_end_to_end_delegated_purchase.1.jsoncontracts/tests/test_snapshots/test_permission_checked_before_escrow_fund_succeeds.1.jsontests/unit/package.jsontests/unit/src/gateway-auth-handlers.test.jstests/unit/src/queue-job-status.test.jstests/unit/src/rateLimit.test.js
💤 Files with no reviewable changes (1)
- apps/backend/orchestrator/src/index.ts
Comment on lines
+199
to
215
| // Acquire lock with retry | ||
| let lockAcquired = false; | ||
| const maxRetries = 20; | ||
| const retryDelay = 50; // 50ms | ||
|
|
||
| for (let attempt = 0; attempt < maxRetries; attempt++) { | ||
| const res = await redis.set(lockKey, "locked", "PX", lockTimeout, "NX"); | ||
| if (res) { | ||
| lockAcquired = true; | ||
| break; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, retryDelay)); | ||
| } | ||
|
|
||
| if (!lockAcquired) { | ||
| throw new Error("Failed to acquire sequence reservation lock"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Retry budget (~1s) is much smaller than lockTimeout (5s), and fixed delay causes synchronized retries.
The total wait before giving up is maxRetries * retryDelay ≈ 20 * 50ms = 1000ms, but the lock holder can legitimately hold the lock for up to lockTimeout (5000ms) — the critical section performs horizonServer.loadAccount(address) (a network call) while holding it. Under normal contention a waiter can exhaust its ~1s budget and throw "Failed to acquire sequence reservation lock" even though the holder is progressing normally, which will surface as spurious reservation failures.
Two issues:
- The retry budget should be aligned with the realistic worst-case hold time (≈
lockTimeout), not ~1/5 of it. - The fixed
retryDelaywith no jitter causes concurrent waiters to retry in lockstep (thundering herd). Adding jitter/backoff spreads contention. The loop also sleeps once more after the final failed attempt.
♻️ Suggested retry tuning with jitter and no trailing sleep
// Acquire lock with retry
let lockAcquired = false;
- const maxRetries = 20;
- const retryDelay = 50; // 50ms
+ const maxRetries = 100;
+ const retryDelay = 50; // base 50ms; total budget ~lockTimeout
for (let attempt = 0; attempt < maxRetries; attempt++) {
const res = await redis.set(lockKey, "locked", "PX", lockTimeout, "NX");
if (res) {
lockAcquired = true;
break;
}
- await new Promise((resolve) => setTimeout(resolve, retryDelay));
+ // Skip sleeping after the last attempt; add jitter to avoid lockstep retries.
+ if (attempt < maxRetries - 1) {
+ const jitter = Math.floor(Math.random() * retryDelay);
+ await new Promise((resolve) => setTimeout(resolve, retryDelay + jitter));
+ }
}📝 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.
Suggested change
| // Acquire lock with retry | |
| let lockAcquired = false; | |
| const maxRetries = 20; | |
| const retryDelay = 50; // 50ms | |
| for (let attempt = 0; attempt < maxRetries; attempt++) { | |
| const res = await redis.set(lockKey, "locked", "PX", lockTimeout, "NX"); | |
| if (res) { | |
| lockAcquired = true; | |
| break; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, retryDelay)); | |
| } | |
| if (!lockAcquired) { | |
| throw new Error("Failed to acquire sequence reservation lock"); | |
| } | |
| // Acquire lock with retry | |
| let lockAcquired = false; | |
| const maxRetries = 100; | |
| const retryDelay = 50; // base 50ms; total budget ~lockTimeout | |
| for (let attempt = 0; attempt < maxRetries; attempt++) { | |
| const res = await redis.set(lockKey, "locked", "PX", lockTimeout, "NX"); | |
| if (res) { | |
| lockAcquired = true; | |
| break; | |
| } | |
| // Skip sleeping after the last attempt; add jitter to avoid lockstep retries. | |
| if (attempt < maxRetries - 1) { | |
| const jitter = Math.floor(Math.random() * retryDelay); | |
| await new Promise((resolve) => setTimeout(resolve, retryDelay + jitter)); | |
| } | |
| } | |
| if (!lockAcquired) { | |
| throw new Error("Failed to acquire sequence reservation lock"); | |
| } |
🤖 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 `@apps/backend/wallet/src/queue/txQueue.ts` around lines 199 - 215, The lock
retry loop in txQueue’s sequence reservation is too short relative to the lock
holder’s `lockTimeout`, and its fixed `retryDelay` causes synchronized retries.
Update the retry logic around the `redis.set` lock acquisition so the total wait
budget matches the expected lock hold time (or derives from `lockTimeout`), add
jitter/backoff to the delay, and avoid sleeping after the last failed attempt.
Use the existing lock-acquire block in `txQueue` as the place to tune the
`maxRetries`, `retryDelay`, and failure path.
Comment on lines
+707
to
+974
| "val": { | ||
| "map": [ | ||
| { | ||
| "key": { | ||
| "symbol": "amount" | ||
| }, | ||
| "val": { | ||
| "i128": { | ||
| "hi": 0, | ||
| "lo": 1000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "buyer" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "seller" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "status" | ||
| }, | ||
| "val": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Released" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "token" | ||
| }, | ||
| "val": { | ||
| "address": "CACMVW2KK4H5FZDFF2AUCAKQTEJMZZWJUIZF23XMRVYQBSXYLHZ6BKWN" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "unlock_time" | ||
| }, | ||
| "val": { | ||
| "u64": 3600 | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| "ext": "v0" | ||
| }, | ||
| 4095 | ||
| ] | ||
| ], | ||
| [ | ||
| { | ||
| "contract_data": { | ||
| "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", | ||
| "key": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Escrow" | ||
| }, | ||
| { | ||
| "u64": 2 | ||
| } | ||
| ] | ||
| }, | ||
| "durability": "persistent" | ||
| } | ||
| }, | ||
| [ | ||
| { | ||
| "last_modified_ledger_seq": 0, | ||
| "data": { | ||
| "contract_data": { | ||
| "ext": "v0", | ||
| "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", | ||
| "key": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Escrow" | ||
| }, | ||
| { | ||
| "u64": 2 | ||
| } | ||
| ] | ||
| }, | ||
| "durability": "persistent", | ||
| "val": { | ||
| "map": [ | ||
| { | ||
| "key": { | ||
| "symbol": "amount" | ||
| }, | ||
| "val": { | ||
| "i128": { | ||
| "hi": 0, | ||
| "lo": 1000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "buyer" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "seller" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "status" | ||
| }, | ||
| "val": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Refunded" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "token" | ||
| }, | ||
| "val": { | ||
| "address": "CACMVW2KK4H5FZDFF2AUCAKQTEJMZZWJUIZF23XMRVYQBSXYLHZ6BKWN" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "unlock_time" | ||
| }, | ||
| "val": { | ||
| "u64": 3600 | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| "ext": "v0" | ||
| }, | ||
| 4095 | ||
| ] | ||
| ], | ||
| [ | ||
| { | ||
| "contract_data": { | ||
| "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", | ||
| "key": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Escrow" | ||
| }, | ||
| { | ||
| "u64": 3 | ||
| } | ||
| ] | ||
| }, | ||
| "durability": "persistent" | ||
| } | ||
| }, | ||
| [ | ||
| { | ||
| "last_modified_ledger_seq": 0, | ||
| "data": { | ||
| "contract_data": { | ||
| "ext": "v0", | ||
| "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", | ||
| "key": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Escrow" | ||
| }, | ||
| { | ||
| "u64": 3 | ||
| } | ||
| ] | ||
| }, | ||
| "durability": "persistent", | ||
| "val": { | ||
| "map": [ | ||
| { | ||
| "key": { | ||
| "symbol": "amount" | ||
| }, | ||
| "val": { | ||
| "i128": { | ||
| "hi": 0, | ||
| "lo": 1000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "buyer" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "seller" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "status" | ||
| }, | ||
| "val": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Released" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "token" | ||
| }, | ||
| "val": { | ||
| "address": "CACMVW2KK4H5FZDFF2AUCAKQTEJMZZWJUIZF23XMRVYQBSXYLHZ6BKWN" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "unlock_time" | ||
| }, | ||
| "val": { | ||
| "u64": 3600 | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| "ext": "v0" | ||
| }, | ||
| 4095 | ||
| ] | ||
| ], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'struct EscrowRecord {
$$$
}' --lang rust contracts/escrow/src/lib.rs
rg -n "released_amount|created_at|order_id" contracts/escrow/src/lib.rsRepository: DelegoLabs/Delego
Length of output: 1020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the current EscrowRecord definition and nearby persistence/event code.
sed -n '1,120p' contracts/escrow/src/lib.rs
printf '\n---\n'
sed -n '730,770p' contracts/escrow/src/lib.rs
printf '\n---\n'
sed -n '820,845p' contracts/escrow/src/lib.rs
printf '\n---\n'
sed -n '1048,1105p' contracts/escrow/src/lib.rs
printf '\n=== snapshot field checks ===\n'
# Compare against other snapshot fixtures that include the newer schema fields.
rg -n '"released_amount"|"created_at"|"order_id"|"escrow_id"' contracts/escrow/test_snapshots/integration_tests/test_double_refund_prevention.1.json
printf '\n---\n'
rg -n '"released_amount"|"created_at"|"order_id"|"escrow_id"' contracts/escrow/test_snapshots/integration_tests/test_escrow_events.1.jsonRepository: DelegoLabs/Delego
Length of output: 7205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the contract still uses the old `unlock_time` field name anywhere.
rg -n "unlock_time|timeout_ledger" contracts/escrow/src/lib.rs contracts/escrow/test_snapshots/integration_tests/test_escrow_events.1.jsonRepository: DelegoLabs/Delego
Length of output: 1281
Regenerate this escrow snapshot for the current schema. These records still use unlock_time and omit released_amount, created_at, escrow_id, and order_id, while EscrowRecord now persists timeout_ledger plus those fields. The fixture is stale and will diverge from the test output.
🤖 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 `@contracts/escrow/test_snapshots/integration_tests/test_escrow_events.1.json`
around lines 707 - 974, The escrow snapshot is stale against the current
EscrowRecord schema: these contract_data entries still serialize unlock_time and
status-only payloads, but the persisted shape now includes timeout_ledger,
released_amount, created_at, escrow_id, and order_id. Regenerate the affected
snapshot data in test_escrow_events.1.json using the current escrow event/test
output so the EscrowRecord fixtures match the updated schema and symbols like
contract_data, Escrow, status, and unlock_time are refreshed consistently.
Comment on lines
+459
to
+517
| "val": { | ||
| "map": [ | ||
| { | ||
| "key": { | ||
| "symbol": "amount" | ||
| }, | ||
| "val": { | ||
| "i128": { | ||
| "hi": 0, | ||
| "lo": 1000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "buyer" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "seller" | ||
| }, | ||
| "val": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "status" | ||
| }, | ||
| "val": { | ||
| "vec": [ | ||
| { | ||
| "symbol": "Disputed" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "token" | ||
| }, | ||
| "val": { | ||
| "address": "CACMVW2KK4H5FZDFF2AUCAKQTEJMZZWJUIZF23XMRVYQBSXYLHZ6BKWN" | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "unlock_time" | ||
| }, | ||
| "val": { | ||
| "u64": 3600 | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'struct EscrowRecord {
$$$
}' --lang rust contracts/escrow/src/lib.rsRepository: DelegoLabs/Delego
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## repo files matching escrow/lib and snapshots ##\n'
git ls-files | rg 'contracts/.*/(lib\.rs|test_snapshots/.*\.json)$|EscrowRecord|released_amount' || true
printf '\n## search for EscrowRecord references ##\n'
rg -n "EscrowRecord|released_amount" contracts/escrow -S || true
printf '\n## list snapshot fixtures mentioning released_amount ##\n'
rg -n '"released_amount"' contracts/escrow/test_snapshots -S || trueRepository: DelegoLabs/Delego
Length of output: 29119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## target snapshot section ##\n'
sed -n '430,540p' contracts/escrow/test_snapshots/integration_tests/test_remove_co_admin.1.json
printf '\n## reference snapshot section with released_amount ##\n'
sed -n '430,540p' contracts/escrow/test_snapshots/integration_tests/test_double_refund_prevention.1.jsonRepository: DelegoLabs/Delego
Length of output: 7169
Add released_amount: 0 to this snapshot. EscrowRecord now persists released_amount, and this fixture omits that field while the other escrow snapshots include it.
🤖 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
`@contracts/escrow/test_snapshots/integration_tests/test_remove_co_admin.1.json`
around lines 459 - 517, The EscrowRecord snapshot in this test fixture is
missing the new persisted released_amount field, so update the test snapshot
data to include released_amount: 0 alongside the existing EscrowRecord keys. Use
the EscrowRecord snapshot structure in test_remove_co_admin.1.json as the target
and keep it consistent with the other escrow snapshots that already include
released_amount.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of change
Test plan
pnpm typecheckpnpm testChecklist
Summary by CodeRabbit
New Features
Bug Fixes