Adapt the code to the Protocol 23 #151
Conversation
WalkthroughRefactors ad-hoc event emissions to typed #[contractevent] structs, upgrades Soroban workspace dependencies, switches Rust WASM target/artifact paths to wasm32v1-none, updates deploy/register APIs and budget cost-estimate usage, and updates Makefile, CI, deploy scripts, tests, and integration-test tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Tests / CLI
participant Deployer as Deployer lib
participant Env as Env
participant Contract as Deployed Contract
Note over Deployer,Env: deployment now uses deploy_v2(..., ())
Caller->>Deployer: deploy_*(salt, wasm_hash, init_args)
Deployer->>Env: deploy_v2(wasm_hash, ())
Env-->>Deployer: contract_id
Deployer->>Contract: initialize(init_args)
Contract-->>Deployer: Ok
Deployer-->>Caller: (contract_id, Ok)
sequenceDiagram
autonumber
participant Logic as Contract logic
participant Event as #[contractevent] Type
participant Env as Env
Note over Logic,Event: typed event emission
Logic->>Event: construct Event{ fields... }
Event->>Env: .publish(&env)
Env-->>Logic: event recorded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (12)
.github/workflows/tests.yml (1)
17-27: Please upgradeactions/checkoutto v4.actionlint flags Line 17 because
actions/checkout@v3is now on deprecated Node 16 and will stop receiving updates. Bumping toactions/checkout@v4keeps the workflow on supported infrastructure and future-proofs the job.deploy/scripts/upgrade.sh (1)
33-33: Update path LGTM; quote args for safety.Path switch to wasm32v1-none is correct. Quote args to avoid word-splitting and strange secrets expansion.
Apply:
-POOL_HASH=$(install "../target/wasm32v1-none/release/pool.wasm" $ADMIN_SECRET) +POOL_HASH=$(install "../target/wasm32v1-none/release/pool.wasm" "$ADMIN_SECRET")contracts/pool/src/tests/budget.rs (2)
705-705: Ensure cost-estimate budget actually captures callback costs.Using cost_estimate() without holding/using the same handle can be confusing. Capture it once and read from the same handle.
- env.cost_estimate().budget().reset_unlimited(); + let ce = env.cost_estimate(); + ce.budget().reset_unlimited();Please verify the snapshot records non‑zero cpu/memory after these tests run (to confirm costs are measured, not zeros).
709-710: Read costs from the same cost-estimate handle.Prevents accidental drift if cost_estimate() semantics change.
- let cpu = env.cost_estimate().budget().cpu_instruction_cost(); - let memory = env.cost_estimate().budget().memory_bytes_cost(); + let cpu = ce.budget().cpu_instruction_cost(); + let memory = ce.budget().memory_bytes_cost();Cargo.toml (1)
41-42: Pin git dep by tag LGTM; consider adding rev for reproducibility.Tag is fine; adding a commit
revwould make builds fully reproducible.contracts/pool/src/tests/configure_as_collateral.rs (1)
160-183: Event assertion moved to Map payload—verify key names and avoid order brittleness.
- Confirm the keys "liq_cap", "pen_order", "util_cap", "discount" exactly match those emitted by the contract event (typos will silently fail the equality).
- Map equality can be order-sensitive depending on construction; if the emitter changes insertion order later, this test may become flaky. Consider asserting by decoding/inspecting keys rather than full Map equality, or use typed event parsing if available.
contracts/pool/src/methods/enable_borrowing_on_reserve.rs (1)
22-31: Emit events only on actual state change (optional).Currently an event is published even if the flag value stays the same. To reduce noise and preserve idempotency semantics, early-return when no change is needed.
Apply this diff:
- reserve.configuration.borrowing_enabled = enabled; - write_reserve(env, asset, &reserve); + if reserve.configuration.borrowing_enabled == enabled { + return Ok(()); + } + reserve.configuration.borrowing_enabled = enabled; + write_reserve(env, asset, &reserve);contracts/pool/src/methods/initialize.rs (1)
18-35: InitializedEvent fields sourced from input—consider sourcing from persisted config.If set_pool_configuration mutates or normalizes values, emitting the event from the stored config guarantees alignment with on-chain state.
contracts/pool/src/tests/withdraw.rs (2)
363-375: Map-based event assertion looks correct; optional nits.
- You can use symbol_short! for constant keys to avoid repeated allocations: "to", "asset", "amount".
- Minor: token_address is borrowed in into_val, so no need to wrap with extra parentheses.
379-379: Avoid relying on hard-coded event index.Using events.get(0) is brittle; prefer locating the event by topic symbol (e.g., "reserve_used_as_coll_disabled") to make the test order-independent.
Additionally, please verify that the new event ordering is stable across runs.
contracts/s-token/src/lib.rs (1)
407-413: Avoid overloading TransferEvent for underlying transfersUsing the same TransferEvent for s-token transfers and underlying token transfers can confuse indexers/consumers. Prefer a distinct UnderlyingTransferEvent.
Apply this diff here:
- TransferEvent { - from: current_address, - to: to.clone(), - amount, - } - .publish(&e); + UnderlyingTransferEvent { + from: current_address, + to: to.clone(), + amount, + } + .publish(&e);Add this struct in contracts/s-token/src/event.rs:
#[contractevent(topics=["underlying_transfer"], data_format = "single-value")] pub(crate) struct UnderlyingTransferEvent { #[topic] pub from: Address, #[topic] pub to: Address, pub amount: i128, }contracts/pool/src/event.rs (1)
93-99: ReserveStatusChangedEvent: naming consistency nitConsider aligning topic string with others ("reserve_status_changed") is fine, but ensure downstream consumers expect this exact topic. No change required if tests cover it.
Confirm event topic names remain stable across analytics/indexers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcontracts/pool/src/tests/snapshots/budget_utilization.snapis excluded by!**/*.snap
📒 Files selected for processing (44)
.github/workflows/tests.yml(1 hunks)Cargo.toml(1 hunks)Makefile(2 hunks)contracts/common-token/Cargo.toml(1 hunks)contracts/common/Cargo.toml(1 hunks)contracts/debt-token/Cargo.toml(1 hunks)contracts/debt-token/src/event.rs(1 hunks)contracts/debt-token/src/lib.rs(6 hunks)contracts/debt-token/src/test.rs(2 hunks)contracts/deployer/src/lib.rs(3 hunks)contracts/deployer/src/test.rs(3 hunks)contracts/pool/Cargo.toml(1 hunks)contracts/pool/src/event.rs(1 hunks)contracts/pool/src/methods/borrow.rs(2 hunks)contracts/pool/src/methods/configure_as_collateral.rs(2 hunks)contracts/pool/src/methods/deposit.rs(2 hunks)contracts/pool/src/methods/enable_borrowing_on_reserve.rs(2 hunks)contracts/pool/src/methods/flash_loan.rs(2 hunks)contracts/pool/src/methods/initialize.rs(2 hunks)contracts/pool/src/methods/liquidate.rs(3 hunks)contracts/pool/src/methods/repay.rs(2 hunks)contracts/pool/src/methods/set_reserve_status.rs(2 hunks)contracts/pool/src/methods/withdraw.rs(2 hunks)contracts/pool/src/tests/borrow.rs(2 hunks)contracts/pool/src/tests/budget.rs(1 hunks)contracts/pool/src/tests/configure_as_collateral.rs(2 hunks)contracts/pool/src/tests/deposit.rs(2 hunks)contracts/pool/src/tests/flash_loan.rs(2 hunks)contracts/pool/src/tests/init_reserve.rs(1 hunks)contracts/pool/src/tests/liquidate.rs(2 hunks)contracts/pool/src/tests/repay.rs(2 hunks)contracts/pool/src/tests/rounding.rs(2 hunks)contracts/pool/src/tests/set_as_collateral.rs(2 hunks)contracts/pool/src/tests/set_reserve_status.rs(2 hunks)contracts/pool/src/tests/sut.rs(6 hunks)contracts/pool/src/tests/withdraw.rs(2 hunks)contracts/pool/src/types/user_configurator.rs(3 hunks)contracts/s-token/Cargo.toml(1 hunks)contracts/s-token/src/event.rs(1 hunks)contracts/s-token/src/lib.rs(9 hunks)contracts/s-token/src/tests/test.rs(4 hunks)deploy/scripts/deploy.sh(1 hunks)deploy/scripts/upgrade.sh(1 hunks)integration-tests/tests/pool.sut.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
contracts/pool/src/tests/borrow.rs (1)
contracts/pool/src/types/price_provider.rs (1)
new(22-30)
contracts/pool/src/tests/repay.rs (2)
contracts/pool/src/types/price_provider.rs (1)
new(22-30)contracts/pool/src/tests/sut.rs (2)
debt_token(590-592)debt_token(614-616)
contracts/pool/src/tests/liquidate.rs (1)
contracts/pool/src/types/price_provider.rs (1)
new(22-30)
contracts/pool/src/tests/deposit.rs (1)
contracts/pool/src/types/price_provider.rs (1)
new(22-30)
contracts/pool/src/methods/flash_loan.rs (2)
contracts/pool/src/methods/utils/validation.rs (1)
require_not_in_grace_period(160-167)contracts/pool/src/storage.rs (6)
read_reserve(54-61)read_token_balance(197-210)read_token_total_supply(175-180)add_protocol_fee_vault(248-257)read_pause_info(139-150)read_pool_config(267-274)
contracts/debt-token/src/lib.rs (1)
contracts/common-token/src/storage.rs (1)
write_metadata(135-138)
contracts/s-token/src/tests/test.rs (1)
contracts/debt-token/src/test.rs (1)
create_token(11-30)
deploy/scripts/upgrade.sh (1)
deploy/scripts/deploy.sh (1)
install(18-26)
contracts/pool/src/tests/flash_loan.rs (1)
contracts/pool/src/types/price_provider.rs (1)
new(22-30)
contracts/pool/src/methods/repay.rs (1)
contracts/pool/src/storage.rs (8)
read_token_balance(197-210)read_token_total_supply(175-180)write_token_balance(212-229)write_token_total_supply(182-195)add_protocol_fee_vault(248-257)add_token_balance(158-173)read_pause_info(139-150)read_pool_config(267-274)
contracts/pool/src/methods/liquidate.rs (1)
contracts/pool/src/storage.rs (4)
add_protocol_fee_vault(248-257)add_token_balance(158-173)read_pause_info(139-150)read_pool_config(267-274)
contracts/pool/src/tests/configure_as_collateral.rs (1)
contracts/pool/src/types/price_provider.rs (1)
new(22-30)
contracts/pool/src/methods/set_reserve_status.rs (1)
contracts/pool/src/storage.rs (2)
read_reserve(54-61)write_reserve(63-68)
contracts/pool/src/methods/withdraw.rs (3)
contracts/pool/src/methods/utils/get_collat_coeff.rs (3)
get_collat_coeff(9-34)get_compounded_amount(36-63)get_lp_amount(66-109)contracts/pool/src/methods/utils/validation.rs (1)
require_gte_initial_health(122-150)contracts/pool/src/storage.rs (1)
read_pause_info(139-150)
contracts/pool/src/tests/sut.rs (1)
contracts/pool/src/tests/budget.rs (1)
price_feed(302-311)
🪛 GitHub Actions: Linter and tests
Makefile
[error] 1-1: Command failed: make clippy. The clippy check failed during the CI run.
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (89)
contracts/pool/src/tests/set_as_collateral.rs (2)
219-231: Lifetimes aligned with the new Sut contractPassing
&Envthrough and returningSut<'_>keeps the helper consistent with the updated pool scaffolding while preserving the existing test flow. No issues spotted.
234-255: Borrowed Env plumbing looks solidChaining through to
init(env)and propagating theSut<'_>lifetime ensures the debt-setup helper stays compatible with the refactored clients without altering behavior. Looks good.contracts/deployer/src/lib.rs (3)
25-28: Protocol 23 deploy_v2 migration looks correctSwitching to
deploy_v2with the empty metadata payload keeps the deployment semantics aligned with the olddeploycall for Protocol 23. No further changes needed here.
48-51: Consistent deploy_v2 usage for s-tokenThis mirrors the pool deployment update and keeps behavior consistent while adopting the new API. All good.
77-80: Debt-token deploy_v2 update LGTMThe debt-token deployment now matches the new Protocol 23 expectations; no issues spotted.
contracts/pool/src/tests/repay.rs (1)
187-194: Event payload assertion matches the new typed event contract.The map keys and value conversions line up with the RepayEvent structure, so this test now accurately validates the emitted data.
contracts/s-token/Cargo.toml (1)
6-6: Non-publishable flag aligns with workspace convention.Marking this contract crate as non-publishable matches the rest of the workspace and avoids accidental publishes. Looks good.
contracts/pool/Cargo.toml (1)
6-6: Non-publishable flag looks good.Consistent with the other contract crates—no issues spotted.
contracts/common/Cargo.toml (1)
6-10: Workspace dependency + publish flag are correct.Disabling publishing and drawing
soroban-fixed-point-mathfrom the workspace are both in line with the migration plan.deploy/scripts/deploy.sh (1)
4-4: Artifact path update matches the new target triple.The deploy script now points at the wasm32v1-none output directory, so deployments will pick up the freshly built artifacts. Looks correct.
contracts/common-token/Cargo.toml (1)
6-6: Non-publishable flag is consistent.Mirrors the other contract crates—no concerns here.
Makefile (2)
19-21: Build step matches the wasm32v1-none rollout.Switching both the build target and artifact listing to the new triple keeps the tooling consistent. All good.
39-40: Check target update looks good.
cargo checknow runs against wasm32v1-none, aligning with the rest of the build. No issues spotted.contracts/debt-token/Cargo.toml (1)
6-6: Non-publishable flag confirmed.Matches the broader policy for contract crates—looks good.
Cargo.toml (1)
35-39: SDK upgrade to 23.0.2 looks good.Matches Protocol 23 migration; no issues spotted.
contracts/deployer/src/test.rs (5)
11-20: WASM imports updated to wasm32v1-none are correct.Imports align with the new target; good.
25-25: New registration API usage LGTM.
env.register(Deployer, ())is the correct Protocol 23 pattern.
64-64: Budget reset path migrated correctly.
env.cost_estimate().budget().reset_default()aligns with the new API.
70-72: Asset registration v2 usage LGTM.
register_stellar_asset_contract_v2(...).address()is the right flow.
95-95: Consistent budget reset after deployments.No issues.
contracts/debt-token/src/event.rs (5)
1-12: Typed events for initialize look correct.Topics and map data are appropriate for Protocol 23.
14-19: SetAuthorizedEvent definition LGTM.Single-value payload with topic id is correct.
21-29: MintEvent definition LGTM.Admin and to as topics; amount as data matches single-value format.
30-36: BurnEvent definition LGTM.
37-43: ClawbackEvent definition LGTM.contracts/debt-token/src/lib.rs (7)
11-12: Event imports are correct.Matches the new typed events module.
53-59: Decimals propagation LGTM.Reads decimals from underlying and stores in metadata correctly.
64-72: InitializedEvent publish LGTM.Fields align with event definition.
155-156: BurnEvent publish LGTM.
178-179: SetAuthorizedEvent publish LGTM.
199-205: MintEvent publish LGTM.Includes admin topic from verified pool.
226-227: ClawbackEvent publish LGTM.contracts/pool/src/tests/flash_loan.rs (2)
5-5: Added Map/Val imports for structured event payloads LGTM.
220-227: Event assertion migrated to Map payload correctly.Keys and values match the new event format.
Please confirm the event struct in pool emits these keys ("amount", "premium", "borrow") to keep tests stable.
contracts/pool/src/tests/deposit.rs (2)
5-5: Added Map/Val imports LGTM.
240-251: Deposit event assertion migrated to Map payload correctly.Matches the new typed event structure.
contracts/pool/src/methods/repay.rs (2)
8-8: Import correctly updated for typed event system.The import change from generic event module to
RepayEventstruct aligns with Protocol 23's migration to typed contract events.
172-177: Event emission properly migrated to structured format.The transition from direct event function calls to structured event publication using
.publish(env)is consistent with Protocol 23's typed contract event system. TheRepayEventstruct maintains the same event parameters (who, asset, amount) while providing type safety and better tooling support.contracts/pool/src/types/user_configurator.rs (2)
5-5: Event imports aligned with typed event system.The import of
ReserveUsedAsCollDisabledEventandReserveUsedAsCollEnabledEventfollows the consistent pattern of migrating to structured event types across the codebase.
50-54: Structured event emission implemented correctly.Both
withdrawanddepositmethods properly emit typed events with appropriate parameters. The structured approach improves type safety and maintains consistent event schemas.Also applies to: 78-82
contracts/pool/src/tests/borrow.rs (2)
5-5: Test imports updated for structured event data.The addition of
MapandValimports supports the new Map-based event payload format that accompanies the structured event system.
372-379: Test payload structure updated for typed events.The test now expects a Map-based payload structure instead of a flat tuple, reflecting the structured event format introduced in Protocol 23. This change ensures tests remain compatible with the new event emission system while maintaining the same validation logic.
contracts/pool/src/tests/init_reserve.rs (1)
86-86: Contract registration updated to new SDK API.The change from
register_contract(None, LendingPool)toregister(LendingPool {}, ())aligns with Protocol 23's updated contract registration API. This reflects broader test infrastructure changes to support the new environment setup.contracts/pool/src/tests/rounding.rs (1)
25-25: Budget access API updated correctly.The migration from
env.budget().reset_unlimited()toenv.cost_estimate().budget().reset_unlimited()follows the updated Soroban SDK budget access pattern. This change maintains the same functionality while using the new API structure.Also applies to: 91-91
integration-tests/tests/pool.sut.ts (1)
543-543: WASM target path updated for Protocol 23.The path change from
wasm32-unknown-unknowntowasm32v1-nonetarget aligns with Protocol 23's updated WASM target requirements. This ensures compatibility with the new runtime environment.contracts/pool/src/methods/borrow.rs (2)
10-10: Event import migrated to typed system.The import change to
BorrowEventfrom the generic event module is consistent with the Protocol 23 structured event pattern.
162-167: Borrow event emission properly structured.The event emission follows the same pattern as other pool methods, using structured
BorrowEventwith.publish(env). This maintains event data integrity while improving type safety.contracts/pool/src/methods/withdraw.rs (2)
2-2: Imports correctly updated for Protocol 23.The addition of
WithdrawEventimport andread_pause_infoaligns with the structured event system and provides necessary functionality for the withdraw operation.Also applies to: 6-6
205-211: Withdraw event properly structured.The
WithdrawEventemission includes all relevant parameters (who, asset, to, amount) and follows the consistent structured pattern across pool methods. The event maintains full traceability of withdrawal operations.contracts/pool/src/tests/liquidate.rs (2)
7-7: Event test payload migration correctly updatedThe import addition of
MapandValfromsoroban_sdkis appropriate for the new contract event structure. Protocol 23 introduces unified event streams between Classic operations and Soroban contracts, with events now supporting map-based data fields.
794-807: Event payload structure correctly adapted to Protocol 23 formatThe test properly constructs the event payload using
Map::<Symbol, Val>::from_array()with structured keys"covered_debt"and"liquidated_collateral". This aligns with the new typed contract event system where events are emitted as structured data rather than simple tuples. The change correctly reflects the migration from function-based event emission to structured contract events.contracts/pool/src/methods/configure_as_collateral.rs (2)
5-5: Correct import for typed contract eventsThe import change from generic event module to
CollatConfigChangeEventstruct is appropriate for the Protocol 23 migration to typed contract events.
26-33: Proper migration to typed contract event publishingThe refactoring correctly replaces direct event function calls with structured event construction and publishing. The
CollatConfigChangeEventstruct properly captures all relevant parameters (asset,liq_cap,pen_order,util_cap,discount) and uses the new.publish(env)pattern consistent with Protocol 23 event system changes.contracts/pool/src/methods/set_reserve_status.rs (2)
4-7: Correctly updated imports for typed eventsThe import restructuring properly imports
ReserveStatusChangedEventfor the new contract event system while maintaining necessary storage functions.
19-23: Proper event publishing migrationThe code correctly constructs
ReserveStatusChangedEventwithassetandactivatedfields and publishes it using.publish(env). This follows the Protocol 23 pattern of typed contract events rather than direct function-based event emissions.contracts/debt-token/src/test.rs (2)
14-14: Contract registration updated to new APIThe migration from
register_contract(None, DebtToken {})toregister(DebtToken {}, ())follows the new deployment API pattern introduced in Protocol 23. This is a correct adaptation to the updated Soroban SDK registration methods.Also applies to: 38-38
18-20: Stellar asset registration properly migrated to v2 APIThe change from older registration patterns to
register_stellar_asset_contract_v2()with.address()extraction correctly adapts to the new Protocol 23 asset contract registration API. Protocol 23 introduces support for the new wasm32v1-none target and updated contract deployment APIs.Also applies to: 42-44
contracts/pool/src/methods/liquidate.rs (2)
9-9: Import correctly updated for typed eventsThe addition of
LiquidationEventimport is appropriate for the Protocol 23 event system migration.
60-65: Event emission properly migrated to structured formatThe liquidation event is correctly constructed using
LiquidationEventstruct withwho,covered_debt, andliquidated_collateralfields, then published via.publish(env). This properly replaces the previous function-based event emission with the new typed contract event system required by Protocol 23.contracts/pool/src/methods/deposit.rs (2)
9-9: Correct event import for Protocol 23The import of
DepositEventreplaces the previous generic event module usage, properly adapting to the typed contract event system.
73-78: Deposit event migration correctly implementedThe code properly constructs
DepositEventwithwho,asset, andamountfields and publishes it using.publish(env). This correctly replaces the previous direct event function call pattern with the new structured event approach required by Protocol 23.contracts/pool/src/tests/set_reserve_status.rs (1)
84-84: Event payload format correctly updated for Protocol 23The test expectations are properly updated to include the event topic as a tuple
(Symbol::new(&env, "reserve_status_changed"), asset)rather than just the asset. This aligns with the new structured contract event format where topics are explicitly defined in the event payload. The change is consistent across both test assertions.Also applies to: 100-100
contracts/pool/src/methods/flash_loan.rs (2)
8-8: Flash loan event import correctly addedThe import of
FlashLoanEventfollows the Protocol 23 pattern for typed contract events.
123-131: Flash loan event properly migrated to structured formatThe
FlashLoanEventis correctly constructed with all necessary fields (who,receiver,asset,amount,premium,borrow) and published using.publish(env). The event captures all the relevant flash loan parameters and follows the new Protocol 23 typed event pattern.contracts/pool/src/tests/configure_as_collateral.rs (1)
7-7: Import additions look correct.Including Map and Val aligns with the new Map-based event payloads in tests.
contracts/pool/src/methods/enable_borrowing_on_reserve.rs (1)
4-4: Typed event imports LGTM.Matches the migration to struct-based events with .publish(env).
contracts/pool/src/methods/initialize.rs (1)
5-5: Import switch to InitializedEvent LGTM.contracts/pool/src/tests/withdraw.rs (1)
5-5: Import additions LGTM.Map and Val are consistent with the new event payload format.
contracts/pool/src/tests/sut.rs (3)
21-40: Target switch to wasm32v1-none paths LGTM; verify CI artifact paths.The new contractimport! file paths look consistent. Ensure your build produces artifacts under target/wasm32v1-none/release in CI.
48-53: Migration to register/register_stellar_asset_contract_v2 LGTM.
- Using address() for TokenClient/TokenAdminClient is correct.
- e.register(...) usage for pool/s-token/debt-token/price-feed/flash-loan-receiver aligns with the new APIs.
Also applies to: 63-66, 100-101, 117-117, 130-130, 134-134
138-138: Budget API change—confirm intended semantics.env.cost_estimate().budget().reset_unlimited() differs from the old env.budget().reset_unlimited(). Confirm this is the desired mechanism for unbounded execution in tests under Protocol 23.
contracts/s-token/src/lib.rs (7)
75-83: Event migration looks correct for initialize()InitializedEvent fields and publish site match storage writes and underlying token decimals.
130-139: ApproveEvent emission aligns with new storage writeTopics and payload are coherent; no behavior change.
249-254: ClawbackEvent emission matches state mutationEvent mirrors spend_balance and total_supply decrement.
272-273: SetAuthorizedEvent publish looks goodEvent carries id and authorize; consistent with write_authorization.
292-298: MintEvent uses pool as admin — goodCorrectly captures caller pool and minted amount.
319-324: BurnEvent mirrors burn amountEvent corresponds to SToken burn path; OK.
458-464: TransferEvent emission in do_transfer() is correctPublished after state updates and optional finalize_transfer; good placement.
contracts/s-token/src/event.rs (3)
13-20: Single-value format for TransferEvent is appropriateTwo topics (from, to) with a single data field (amount) keeps events compact.
52-61: InitializedEvent payload looks consistentTopics (underlying_asset, pool) and map fields (decimals, name, symbol) are aligned with initialize().
1-11: No changes needed forcontracteventdata_format
Thedata_format = "map"option is valid (and the default) in the pinned Soroban SDK.contracts/s-token/src/tests/test.rs (3)
18-27: WASM imports updated to wasm32v1-none — goodPaths and targets match Protocol 23 expectations for tests.
40-46: Switch to register/register_stellar_asset_contract_v2 is correctConstructing clients from returned addresses is the new pattern; looks good.
If CI builds only the s-token crate, ensure workspace-level build includes pool, debt_token, and oracle so their WASM artifacts exist at ../../target/wasm32v1-none/release.
71-73: Budget reset via cost_estimate() is correctMatches updated API.
contracts/pool/src/event.rs (5)
3-22: Pool InitializedEvent fields cover prior config fullyTopics and data fields mirror initialize() inputs; good migration.
24-36: reserve_used_as_coll_ events are compact and clear*Single-value format with who as topic is sensible.
38-69: Deposit/Borrow/Repay events use map — consistentFields and types align with expected semantics.
71-80: CollatConfigChangeEvent covers all parametersNaming and types match config fields.
108-119: FlashLoanEvent includes who, receiver, asset as topics — good for filteringPayload captures amount, premium, borrow flag. Looks correct.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contracts/pool/src/tests/snapshots/budget_utilization.snapis excluded by!**/*.snapintegration-tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
README.md(1 hunks)integration-tests/package.json(1 hunks)integration-tests/tests/pool.sut.ts(8 hunks)integration-tests/tests/pool/7.liquidate.budget.spec.ts(1 hunks)integration-tests/tests/soroban.client.ts(4 hunks)integration-tests/tests/soroban.config.ts(1 hunks)integration-tests/tests/soroban.converter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration-tests/tests/soroban.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- integration-tests/tests/pool.sut.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (3)
integration-tests/tests/pool/7.liquidate.budget.spec.ts (1)
1-1: Scoped SDK import looks goodThe switch to the scoped
@stellar/stellar-sdkimport keeps the test suite aligned with the new dependency layout. Nicely done.integration-tests/tests/soroban.converter.ts (1)
1-1: Converter now points at the scoped SDKImporting
Addressandxdrfrom@stellar/stellar-sdkmatches the package update. All good here.integration-tests/tests/soroban.client.ts (1)
1-132: RPC migration looks consistentThe client now relies on the
rpcnamespace throughout, and the simulation/assemble flow matches the Protocol 23 SDK. Looks solid.
integration-tests/package.json
Outdated
| "@types/chai": "^5.2.2", | ||
| "@types/chai-as-promised": "^8.0.2", | ||
| "@types/mocha": "^10.0.10", | ||
| "@types/node": "^24.5.2", | ||
| "@stellar/stellar-sdk": "14.2.0", | ||
| "bigint-conversion": "^2.4.3", | ||
| "bignumber.js": "^9.3.1", | ||
| "chai": "^6.0.1", | ||
| "chai-as-promised": "^8.0.2", | ||
| "cross-env": "^10.0.0", | ||
| "dotenv": "^17.2.2", | ||
| "mocha": "^11.7.2", | ||
| "ts-node": "^10.9.2", | ||
| "typescript": "^5.9.2" |
There was a problem hiding this comment.
Fix SDK version to a published release
@stellar/[email protected] hasn’t been published, so installs will fail and the integration tests can’t run. Please pin to the latest available release (14.1.1 today) or whichever version is actually published before merging.
- "@stellar/stellar-sdk": "14.2.0",
+ "@stellar/stellar-sdk": "14.1.1",Based on learnings.
📝 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.
| "@types/chai": "^5.2.2", | |
| "@types/chai-as-promised": "^8.0.2", | |
| "@types/mocha": "^10.0.10", | |
| "@types/node": "^24.5.2", | |
| "@stellar/stellar-sdk": "14.2.0", | |
| "bigint-conversion": "^2.4.3", | |
| "bignumber.js": "^9.3.1", | |
| "chai": "^6.0.1", | |
| "chai-as-promised": "^8.0.2", | |
| "cross-env": "^10.0.0", | |
| "dotenv": "^17.2.2", | |
| "mocha": "^11.7.2", | |
| "ts-node": "^10.9.2", | |
| "typescript": "^5.9.2" | |
| "@types/chai": "^5.2.2", | |
| "@types/chai-as-promised": "^8.0.2", | |
| "@types/mocha": "^10.0.10", | |
| "@types/node": "^24.5.2", | |
| "@stellar/stellar-sdk": "14.1.1", | |
| "bigint-conversion": "^2.4.3", | |
| "bignumber.js": "^9.3.1", | |
| "chai": "^6.0.1", | |
| "chai-as-promised": "^8.0.2", | |
| "cross-env": "^10.0.0", | |
| "dotenv": "^17.2.2", | |
| "mocha": "^11.7.2", | |
| "ts-node": "^10.9.2", | |
| "typescript": "^5.9.2" |
🤖 Prompt for AI Agents
In integration-tests/package.json around lines 16 to 29, the dependency
@stellar/stellar-sdk is set to 14.2.0 which is not published and breaks
installs; update the package.json to pin @stellar/stellar-sdk to a published
release (e.g., "14.1.1") or any currently published version, run npm install (or
yarn) to verify resolution, and update package-lock.json/yarn.lock accordingly
before committing.
| To run the tests you need to install **soroban-cli** version 20.0.2 | ||
|
|
||
| ```shell | ||
| cargo install --locked --version 20.1.1 soroban-cli --features opt | ||
| cargo install --locked stellar-cli@23.1.3 --features opt | ||
| ``` |
There was a problem hiding this comment.
Fix CLI name/version inconsistency
Line 29 still tells readers to install soroban-cli 20.0.2, but the command below now installs [email protected]. This contradicts itself and will confuse anyone following the setup steps. Please align the prose with the actual command.
-To run the tests you need to install **soroban-cli** version 20.0.2
+To run the tests you need to install **stellar-cli** version 23.1.3📝 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.
| To run the tests you need to install **soroban-cli** version 20.0.2 | |
| ```shell | |
| cargo install --locked --version 20.1.1 soroban-cli --features opt | |
| cargo install --locked [email protected] --features opt | |
| ``` | |
| To run the tests you need to install **stellar-cli** version 23.1.3 | |
🤖 Prompt for AI Agents
In README.md around lines 29 to 33, the text says to install soroban-cli version
20.0.2 but the command installs [email protected]; update the prose to match
the actual install command (or alternatively change the command to install
[email protected] if that was intended). Ensure the line clearly states the
correct CLI name and version that the subsequent shell command will install and
keep the version number consistent between the sentence and the command.
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 (3)
integration-tests/tests/soroban.converter.ts (2)
70-75: Guard TransactionMeta version; unguardedmeta.v4()can throw on non‑v4 transactions.On networks/artifacts not yet at the targeted protocol,
meta.v4()will raise. Recommend handling v4 with a safe fallback to v3 (and a clear error otherwise).Apply:
-export function parseMetaXdrToJs<T>(meta: xdr.TransactionMeta): T { - const value = meta.v4() - .sorobanMeta() - .returnValue(); - - return parseScvToJs(value); -} +export function parseMetaXdrToJs<T>(meta: xdr.TransactionMeta): T { + // Be resilient to v3/v4 TransactionMeta during protocol transitions. + let value: xdr.ScVal; + try { + value = (meta as any).v4().sorobanMeta().returnValue(); + } catch { + try { + value = (meta as any).v3().sorobanMeta().returnValue(); + } catch { + throw new Error(`Unsupported TransactionMeta arm: ${(meta.switch() as any)?.name}`); + } + } + return parseScvToJs(value); +}If you prefer strict typing, switch on
meta.switch()against the sdk’sTransactionMetaTypeenum for v4/v3 instead of try/catch (names vary slightly across sdk gens—verify in your installed version).
46-49: Bug:convertToScvU128uses Int64 for the high part (should be Uint64).
xdr.UInt128Parts.hiexpects an unsigned 64‑bit integer. Usingxdr.Int64can corrupt values ≥ 2^63.Apply:
export function convertToScvU128(value: bigint): xdr.ScVal { return xdr.ScVal.scvU128(new xdr.UInt128Parts({ lo: xdr.Uint64.fromString((value & BigInt(0xFFFFFFFFFFFFFFFFn)).toString()), - hi: xdr.Int64.fromString(((value >> BigInt(64)) & BigInt(0xFFFFFFFFFFFFFFFFn)).toString()), + hi: xdr.Uint64.fromString(((value >> BigInt(64)) & BigInt(0xFFFFFFFFFFFFFFFFn)).toString()), })) }integration-tests/tests/pool.sut.ts (1)
211-257: Default asset_decimals for XRP/USDC should be 9 (not 7).Fallbacks of 7 will misconfigure price feed decimals if env vars are absent, diverging from token initialization (XRP/USDC are set to 9 elsewhere). Use 9 as defaults.
Apply:
{ asset: "XRP", - asset_decimals: +(process.env['XRP_DECIMALS'] ?? 7), + asset_decimals: +(process.env['XRP_DECIMALS'] ?? 9), ... }, { asset: "USDC", - asset_decimals: +(process.env['USDC_DECIMALS'] ?? 7), + asset_decimals: +(process.env['USDC_DECIMALS'] ?? 9), ... },
🧹 Nitpick comments (5)
Makefile (1)
39-39: Align check target with configurable triple; consider dropping --release for speed.Use the same $(WASM_TARGET). Release opts don’t affect cargo check and slow CI.
-check: - cargo check --target wasm32v1-none --release +check: + cargo check --target $(WASM_TARGET)CI tip: update any cache keys/paths from target/wasm32-unknown-unknown to target/$(WASM_TARGET) to avoid cache misses.
integration-tests/tests/soroban.converter.ts (1)
56-58: Avoid 53‑bit loss constructing U64 from a JS number.
new xdr.Uint64(value)risks precision loss for values > 2^53‑1. Prefer string‑based ctor.Apply:
export function convertToScvU64(value: number): xdr.ScVal { - return xdr.ScVal.scvU64(new xdr.Uint64(value)); + return xdr.ScVal.scvU64(xdr.Uint64.fromString(String(value))); }integration-tests/tests/pool/1.borrower.spec.ts (3)
250-263: Assert the intended invariant explicitly (pool accounting vs ledger).To make the intent unambiguous, assert that the token ledger at the sToken contract address equals pool sToken balance + protocol fee.
Apply in-place:
assert.equal(sXlmBalance + protocolFeeXlmBalance, 10_000_000n); assert.equal(sXlmTokenBalance, 10_000_000n); + assert.equal(sXlmTokenBalance, sXlmBalance + protocolFeeXlmBalance);
271-283: Mirror the invariant check from Case 7 for full repay.Optional: also fetch sXLM token ledger and assert equality with pool accounting + protocol fee, like in Case 7.
Example:
const borrower1DXlmBalance = await debtTokenBalanceOf(client, "XLM", borrower1Address); const sXlmBalance = await sTokenUnderlyingBalanceOf(client, "XLM"); + const sXlmTokenBalance = await tokenBalanceOf(client, "XLM", process.env.SLENDER_S_TOKEN_XLM); const dXlmSupply = await debtTokenTotalSupply(client, "XLM"); assert(borrower1XlmBalance < 10_000_000n && borrower1XlmBalance > 9_990_000n); assert(protocolFeeXlmBalance > 0 && protocolFeeXlmBalance < 1_000n); assert.equal(borrower1DXlmBalance, 0n); assert(sXlmBalance + protocolFeeXlmBalance > 90_000_000n && sXlmBalance + protocolFeeXlmBalance < 90_010_000n); + assert.equal(sXlmTokenBalance, sXlmBalance + protocolFeeXlmBalance);
291-304: Do the same invariant check for XRP for completeness.Optional: verify ledger vs pool+protocolFee equality for XRP as well.
Example:
const borrower2DXrpBalance = await debtTokenBalanceOf(client, "XRP", borrower2Address); const sXrpBalance = await sTokenUnderlyingBalanceOf(client, "XRP"); + const sXrpTokenBalance = await tokenBalanceOf(client, "XRP", process.env.SLENDER_S_TOKEN_XRP); const dXrpSupply = await debtTokenTotalSupply(client, "XRP"); assert(borrower2XrpBalance < 1_000_000_000n && borrower2XrpBalance > 999_000_000n); assert(protocolFeeXrpBalance > 0 && protocolFeeXrpBalance < 100_000n); assert.equal(borrower2DXrpBalance, 0n); assert(sXrpBalance + protocolFeeXrpBalance > 9_000_000_000n && sXrpBalance + protocolFeeXrpBalance < 9_001_000_000n); + assert.equal(sXrpTokenBalance, sXrpBalance + protocolFeeXrpBalance);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
deploy/artifacts/testnet/contract-bindings.zipis excluded by!**/*.zipdeploy/artifacts/testnet/debt_token.wasmis excluded by!**/*.wasmdeploy/artifacts/testnet/deployer.wasmis excluded by!**/*.wasmdeploy/artifacts/testnet/flash_loan_receiver_mock.wasmis excluded by!**/*.wasmdeploy/artifacts/testnet/pool.wasmis excluded by!**/*.wasmdeploy/artifacts/testnet/price_feed_mock.wasmis excluded by!**/*.wasmdeploy/artifacts/testnet/s_token.wasmis excluded by!**/*.wasmdeploy/artifacts/testnet/token.wasmis excluded by!**/*.wasmintegration-tests/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
Makefile(2 hunks)deploy/artifacts/testnet/.contracts(1 hunks)deploy/scripts/deploy.sh(2 hunks)integration-tests/package.json(1 hunks)integration-tests/tests/pool.sut.ts(10 hunks)integration-tests/tests/pool/1.borrower.spec.ts(5 hunks)integration-tests/tests/soroban.converter.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration-tests/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
integration-tests/tests/pool/1.borrower.spec.ts (1)
integration-tests/tests/pool.sut.ts (5)
tokenBalanceOf(308-320)debtTokenBalanceOf(294-306)sTokenUnderlyingBalanceOf(352-364)protocolFee(366-377)debtTokenTotalSupply(391-401)
integration-tests/tests/pool.sut.ts (2)
integration-tests/tests/soroban.client.ts (1)
SorobanClient(19-132)integration-tests/tests/soroban.converter.ts (5)
convertToScvAddress(9-12)parseScvToJs(77-156)convertToScvU32(52-54)convertToScvU64(56-58)convertToScvI128(38-43)
🪛 Gitleaks (8.28.0)
deploy/scripts/deploy.sh
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
deploy/artifacts/testnet/.contracts
[high] 1-1: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 18-18: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (9)
deploy/scripts/deploy.sh (2)
4-4: Align build path with wasm32v1-none artifactsPointing
BUILDattarget/wasm32v1-none/releasematches the output location produced by the Protocol 23stellar contract build, so the later copy/optimize steps still pick up the compiled WASM.(developers.stellar.org)
20-24: Adopt the non-deprecated WASM upload commandSwitching to
stellar contract uploadkeeps the script current with the CLI (the oldinstallsubcommand is now deprecated) and ensures the function continues to return the WASM hash expected by downstream steps.(developers.stellar.org)deploy/artifacts/testnet/.contracts (1)
1-21: Contract/account IDs flagged as secrets are expected. The values beginning withCandGare Soroban contract IDs and Stellar public account IDs respectively; they are intended to be public and don't expose secret material, so the Gitleaks alerts here are false positives.(stellar.github.io)Makefile (1)
19-20: Make Wasm target configurable and artifact listing robust
- Introduce
WASM_TARGET ?= wasm32v1-noneat the top for easy overrides.- Replace brittle
lson a glob with afind … -execto avoid failures when no artifacts exist.Apply:
WASM_TARGET ?= wasm32v1-none build: - cargo build --target wasm32v1-none --release - @ls -l target/wasm32v1-none/release/*.wasm + cargo build --target $(WASM_TARGET) --release + @find target/$(WASM_TARGET)/release -maxdepth 1 -name '*.wasm' -exec ls -l {} + 2>/dev/null || trueManually verify that the
$(WASM_TARGET)Rust target is installed (e.g., viarustup target list) sincerustup/rustcweren’t available during automated verification.integration-tests/tests/soroban.converter.ts (1)
1-1: Verify imports, dependency versions, and Node.js engine requirements
- Confirm no remaining
stellar-sdkimports in the repo.- Ensure
@stellar/stellar-sdkand@stellar/stellar-baseare both declared at v14.x in package.json.- Verify CI and runtime are configured to use Node.js ≥ 20 (required by SDK v14.x).
integration-tests/tests/pool/1.borrower.spec.ts (1)
460-463: LGTM: full RWA withdraw assertions are precise.The after/before deltas and zeroed in‑pool checks are clear and correct.
integration-tests/tests/pool.sut.ts (3)
366-377: New protocolFee() helper looks good; ensure method name matches contract spec.Interface aligns with other simulate helpers; just confirm pool method is exactly "protocol_fee".
552-559: WASM target path updated — verify build artifact exists.Ensure CI/build produces ../target/wasm32v1-none/release/flash_loan_receiver_mock.wasm before deploy step.
704-714: Budget snapshot fields: confirm cost/readBytes under Protocol 23 SDK.
- simulation["cost"]: confirm property path (might be simulation.cost).
- resources.diskReadBytes(): verify renamed method exists; otherwise fall back to resources.readBytes().
Example defensive tweak if needed:
- cost: transactionResult.simulation["cost"], + cost: (transactionResult.simulation as any).cost ?? transactionResult.simulation["cost"], - readBytes: resources.diskReadBytes(), + readBytes: (resources as any).diskReadBytes ? resources.diskReadBytes() : resources.readBytes(),
| @@ -1,4 +1,4 @@ | |||
| import { Keypair, xdr } from "stellar-sdk"; | |||
| import { Keypair, xdr } from "@stellar/stellar-sdk"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
SDK bump requires Node 20 and base XDR alignment — verify CI/runtime.
Upgrading to @stellar/stellar-sdk v14.x implies Node >= 20 and matching @stellar/stellar-base to avoid XDR mismatches. Please confirm CI and package versions are aligned.
Based on learnings
Run:
🏁 Script executed:
#!/bin/bash
set -e
echo "== Node versions in workflows =="
fd -a .github/workflows 2>/dev/null | xargs -I {} rg -n "setup-node|node-version" {} || true
echo "== Declared SDK/base versions =="
fd -a package.json 2>/dev/null | while read f; do
echo ":: $f"
jq -r '{
sdk: (.dependencies["@stellar/stellar-sdk"] // .devDependencies["@stellar/stellar-sdk"]),
base: (.dependencies["@stellar/stellar-base"] // .devDependencies["@stellar/stellar-base"]),
engines: .engines?
}' "$f"
doneLength of output: 175
Require Node ≥20 in CI and pin matching stellar-base version
integration-tests/package.json uses @stellar/[email protected] without specifying @stellar/stellar-base or a Node engine, and CI workflows don’t pin node-version. To prevent runtime and XDR mismatches, add "engines": { "node": ">=20" } to package.json, pin Node 20.x in your CI workflows, and include @stellar/[email protected] in dependencies.
🤖 Prompt for AI Agents
In integration-tests/tests/pool.sut.ts around lines 1 to 1: CI and package setup
can cause runtime/XDR mismatches because package.json doesn’t declare a Node
engine or pin stellar-base; update integration-tests/package.json to include
"engines": { "node": ">=20" } and add "@stellar/stellar-base": "14.2.0" to
dependencies, and update CI workflow files to pin Node to a 20.x version (e.g.,
node-version: '20') so the test environment and
@stellar/stellar-sdk/@stellar/stellar-base versions are consistent.
|
|
||
| deploy-contracts: | ||
| (cd deploy/artifacts && shopt -s dotglob; rm -rf *) | ||
| (cd deploy/artifacts/$(env) && shopt -s dotglob; rm -rf *) |
There was a problem hiding this comment.
Bash-only shopt under /bin/sh breaks cleanup; also mkdir is missing.
make defaults to /bin/sh; shopt will fail and dotfiles won’t be removed. Ensure the dir exists and use a POSIX-safe deletion.
Apply:
-deploy-contracts:
- (cd deploy/artifacts/$(env) && shopt -s dotglob; rm -rf *)
+deploy-contracts:
+ @mkdir -p deploy/artifacts/$(env)
+ @(cd deploy/artifacts/$(env) && find . -mindepth 1 -exec rm -rf {} +)
./deploy/scripts/deploy.sh $(env)Optional (outside this hunk): set the shell and validate env.
SHELL := /usr/bin/env bash
ifndef env
$(error env is not set. Usage: make <target> env=local|testnet|mainnet)
endif🤖 Prompt for AI Agents
Makefile lines 23: the recipe uses the bash-only "shopt -s dotglob" which will
fail under /bin/sh and also doesn't ensure the target directory exists or create
it before deletion; replace the bash-specific construct with POSIX-safe
commands: ensure the target directory exists with mkdir -p
deploy/artifacts/$(env) and then remove its contents with a POSIX-safe rm -rf
deploy/artifacts/$(env)/* deploy/artifacts/$(env)/.[!.]*
deploy/artifacts/$(env)/..?* (or simply rm -rf deploy/artifacts/$(env)/*
deploy/artifacts/$(env)/.[!.]* deploy/artifacts/$(env)/..?* to avoid deleting
the directory itself), and avoid shopt entirely; optionally, outside this hunk
add a SHELL := /usr/bin/env bash and an env existence check as suggested if you
intentionally require bash.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
deploy/scripts/upgrade.sh (1)
25-30: Command change to "upload" looks correct, but improve error handling.The change from
stellar contract installtostellar contract uploadcorrectly aligns with Protocol 23 and matches the implementation indeploy.sh.However, combining
localdeclaration with command substitution masks the command's exit code. Ifstellar contract uploadfails, the function will still appear to succeed becauselocalalways returns 0.Apply this diff to separate declaration from assignment for proper error propagation:
- local hash=$(stellar contract upload \ + local hash + hash=$(stellar contract upload \ --wasm $1 \ --source $2 \ --rpc-url "$SOROBAN_RPC_URL" \ --network-passphrase "$PASSPHRASE") echo $hashcontracts/s-token/src/lib.rs (3)
249-253: LGTM! ClawbackEvent properly captures the clawback operation.The event correctly includes the
fromaddress andamount. Minor note:from.clone()at line 250 is redundant sincefromwas already cloned at line 246, but this doesn't affect correctness.If desired, you can remove the redundant clone:
ClawbackEvent { - from: from.clone(), + from, amount, } .publish(&e);
407-412: LGTM! TransferEvent properly logged after underlying asset transfer.The event correctly captures the transfer of underlying assets from the contract to the recipient. The ordering (transfer action followed by event emission) is correct. Minor note:
to.clone()at line 409 is redundant sincetowas already borrowed at line 405.If desired, you can remove the redundant clone:
TransferEvent { from: current_address, - to: to.clone(), + to, amount, } .publish(&e);
458-463: LGTM! TransferEvent properly logged in core transfer logic.The event correctly captures the transfer details after balance updates and validation are complete. Minor note: both
from.clone()andto.clone()are redundant since these addresses were already cloned earlier in the function (lines 437-438, 440-441).If desired, you can remove the redundant clones:
TransferEvent { - from: from.clone(), - to: to.clone(), + from, + to, amount, } .publish(e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
deploy/artifacts/mainnet/debt_token.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/debt_token_optimized.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/deployer.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/flash_loan_receiver_mock.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/pool.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/pool_optimized.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/price_feed_mock.wasmis excluded by!**/*.wasmdeploy/artifacts/mainnet/s_token_optimized.wasmis excluded by!**/*.wasm
📒 Files selected for processing (6)
contracts/debt-token/src/lib.rs(7 hunks)contracts/pool/src/lib.rs(1 hunks)contracts/s-token/src/lib.rs(10 hunks)deploy/artifacts/mainnet/.contracts(1 hunks)deploy/scripts/upgrade.sh(1 hunks)integration-tests/tests/pool/1.borrower.spec.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/debt-token/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
deploy/scripts/upgrade.sh (1)
deploy/scripts/deploy.sh (1)
install(18-26)
integration-tests/tests/pool/1.borrower.spec.ts (1)
integration-tests/tests/pool.sut.ts (5)
tokenBalanceOf(308-320)debtTokenBalanceOf(294-306)sTokenUnderlyingBalanceOf(352-364)protocolFee(366-377)debtTokenTotalSupply(391-401)
contracts/s-token/src/lib.rs (1)
contracts/s-token/src/storage.rs (1)
e(47-47)
🪛 Shellcheck (0.11.0)
deploy/scripts/upgrade.sh
[warning] 25-25: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (15)
deploy/artifacts/mainnet/.contracts (1)
4-8: Approve updated contract hashes
SLENDER_S_TOKEN_HASH, SLENDER_DEBT_TOKEN_HASH, and SLENDER_POOL_HASH have been updated to correctly formatted 256-bit hex values consistent with Protocol 23 redeployment. No residual references to previous hashes remain in the codebase. Please confirm these values match the actual deployed mainnet contracts.contracts/pool/src/lib.rs (1)
55-57: LGTM! Version bump to 2 aligns with Protocol 23 and all version checks and tests now reference the updated version.deploy/scripts/upgrade.sh (1)
33-33: WASM target path update verifiedMakefile, CI workflows, and deploy scripts all reference the new wasm32v1-none target path; no further changes required.
integration-tests/tests/pool/1.borrower.spec.ts (5)
14-14: LGTM! Import changes align with Protocol 23 migration.The addition of
protocolFeeimport and removal of treasury-related imports correctly reflect the transition from treasury-based to protocol-fee-based accounting.Also applies to: 26-26
249-262: LGTM! Protocol fee accounting is correctly implemented.The test properly verifies that:
- The s-token contract holds the expected total tokens (line 261)
- The available balance plus protocol fee equals the total repayment (line 260)
- Protocol fees are being accrued as expected (line 257)
271-282: LGTM! Full repayment verification is correct.The assertions properly validate that after full repayment:
- Protocol fees continue to accrue (line 278)
- Total balance (available + protocol fee) matches the expected repayment amount within the interest accrual range (lines 280-281)
- Borrower's debt is cleared (line 279)
291-302: LGTM! XRP repayment validation mirrors XLM logic correctly.The test properly verifies XRP protocol fee accounting with appropriately scaled assertion ranges that match the debt magnitude.
459-462: LGTM! Withdrawal assertions are logically sound.The updated assertions clearly verify full withdrawal behavior:
- Balance increases by the withdrawal amount (line 459)
- In-pool balance is fully depleted (lines 460-461)
Note: There's a typo
borrwer1RWABalanceBefore(missing 'o') that exists throughout the file, but it's used consistently.contracts/s-token/src/lib.rs (7)
4-10: LGTM! Event imports properly structured.The refactored imports correctly bring in the typed event structs from
crate::eventalongside the existing storage module, aligning with the Protocol 23 migration pattern.
75-82: LGTM! InitializedEvent properly captures initialization state.The typed event correctly includes all relevant initialization parameters (underlying_asset, pool, decimals, name, symbol) and uses
.publish(&e)to emit the event.
92-92: LGTM! Version bumped for Protocol 23.The version increment to 2 appropriately reflects the breaking changes introduced by the Protocol 23 event refactor.
132-138: LGTM! ApproveEvent correctly models the approval.The event properly captures the approval details with
torepresenting the spender, consistent with token standard conventions.
272-272: LGTM! SetAuthorizedEvent properly records authorization changes.The event correctly captures the
idandauthorizestatus in a concise manner.
292-297: LGTM! MintEvent properly captures mint authorization.The event correctly records the
admin(pool),torecipient, andamount, providing a complete audit trail for mint operations.
319-323: LGTM! BurnEvent correctly captures the burn amount.The event records the
fromaddress and theamount_to_burn. Note thatamount_to_withdrawis not included in the event, which appears intentional since the event focuses on the burn operation itself rather than the subsequent withdrawal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/pool/src/tests/upgrade.rs (2)
28-30: Rename variable to reflect v100 artifact.The variable
pool_v2_wasmreferences a v100 artifact but retains the old v2 naming, which is misleading.Apply this diff to improve clarity:
- let pool_v2_wasm = env.deployer().upload_contract_wasm(pool_v100::WASM); + let pool_v100_wasm = env.deployer().upload_contract_wasm(pool_v100::WASM); - sut.pool.upgrade(&pool_v2_wasm); + sut.pool.upgrade(&pool_v100_wasm);And update the assertion references:
vec![&env, pool_v2_wasm.into_val(&env)] + vec![&env, pool_v100_wasm.into_val(&env)]
56-58: Rename variables to reflect v100 artifacts.The variables
pool_v2_wasm,s_token_v2_wasm, anddebt_token_v2_wasmreference v100 artifacts but retain the old v2 naming.Apply this diff to improve clarity:
- let pool_v2_wasm = env.deployer().upload_contract_wasm(pool_v100::WASM); - let s_token_v2_wasm = env.deployer().upload_contract_wasm(s_token_v100::WASM); - let debt_token_v2_wasm = env.deployer().upload_contract_wasm(debt_token_v100::WASM); + let pool_v100_wasm = env.deployer().upload_contract_wasm(pool_v100::WASM); + let s_token_v100_wasm = env.deployer().upload_contract_wasm(s_token_v100::WASM); + let debt_token_v100_wasm = env.deployer().upload_contract_wasm(debt_token_v100::WASM);And update all usage sites at lines 64-66:
- sut.pool.upgrade_token(&asset, &true, &s_token_v2_wasm); - sut.pool.upgrade_token(&asset, &false, &debt_token_v2_wasm); - sut.pool.upgrade(&pool_v2_wasm); + sut.pool.upgrade_token(&asset, &true, &s_token_v100_wasm); + sut.pool.upgrade_token(&asset, &false, &debt_token_v100_wasm); + sut.pool.upgrade(&pool_v100_wasm);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
mocks/debt_token_v100_mock.wasmis excluded by!**/*.wasmmocks/debt_token_v2_mock.wasmis excluded by!**/*.wasmmocks/pool_v100_mock.wasmis excluded by!**/*.wasmmocks/pool_v2_mock.wasmis excluded by!**/*.wasmmocks/s_token_v100_mock.wasmis excluded by!**/*.wasmmocks/s_token_v2_mock.wasmis excluded by!**/*.wasm
📒 Files selected for processing (2)
contracts/pool/src/tests/budget.rs(5 hunks)contracts/pool/src/tests/upgrade.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/pool/src/tests/budget.rs (4)
contracts/debt-token/src/lib.rs (1)
upgrade(84-88)contracts/pool/src/lib.rs (1)
upgrade(42-44)contracts/s-token/src/lib.rs (1)
upgrade(85-89)interfaces/pool-interface/src/lib.rs (1)
upgrade(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (7)
contracts/pool/src/tests/upgrade.rs (2)
10-20: LGTM! Mock imports updated to v100.The module imports have been correctly updated from v2 to v100, with proper paths to the v100 mock WASM files.
72-77: LGTM! Version assertions updated correctly.The version checks have been properly updated from the old v1/v2 versions (1, 2) to the new Protocol 23 versions (2, 100), consistent with the mock artifact changes.
contracts/pool/src/tests/budget.rs (5)
24-24: LGTM! Import updated to v100.The import statement has been correctly updated to reference v100 mock artifacts.
647-650: Rename variable to reflect v100 artifact.The variable
pool_v100_wasmis correctly named and the upgrade flow looks good.
662-665: Rename variable to reflect v100 artifact.The variable
s_token_v100_wasmis correctly named and the upgrade flow looks good.
675-681: LGTM! Debt token upgrade flow updated correctly.The variable naming and upgrade flow are correct, using the v100 artifact.
705-734: LGTM! Budget measurement adapted to Protocol 23 API.The
measure_budgetfunction has been correctly updated to use the newcost_estimate().budget()API:
- Resets budget to unlimited before the callback
- Reads CPU and memory costs from the cost estimate after callback execution
- The approach simplifies budget tracking by removing manual delta calculations
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
integration-tests/tests/pool.sut.ts (1)
1-1: SDK bump already flagged in past review.The import of
@stellar/stellar-sdkv14.2.0 and Node ≥20 requirement was already flagged in the past review comment. Please address that concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/pool/src/tests/snapshots/budget_utilization.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
integration-tests/tests/pool.sut.ts(17 hunks)integration-tests/tests/pool/2.liquidator.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
integration-tests/tests/pool.sut.ts (3)
integration-tests/tests/soroban.client.ts (1)
SorobanClient(19-132)integration-tests/tests/soroban.converter.ts (7)
convertToScvAddress(9-12)parseScvToJs(77-156)convertToScvBytes(64-67)convertToScvMap(18-27)convertToScvU32(52-54)convertToScvU64(56-58)convertToScvI128(38-43)integration-tests/tests/soroban.config.ts (1)
adminKeys(8-8)
integration-tests/tests/pool/2.liquidator.spec.ts (2)
integration-tests/tests/pool.sut.ts (7)
borrow(403-420)tokenBalanceOf(308-320)debtTokenBalanceOf(294-306)sTokenUnderlyingBalanceOf(352-364)debtTokenTotalSupply(391-401)accountPosition(322-333)liquidate(480-495)contracts/pool/src/lib.rs (2)
borrow(148-150)liquidate(164-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (9)
integration-tests/tests/pool.sut.ts (6)
83-83: LGTM: isRelease flag correctly differentiates test vs. release configurations.The
falseparameter in the standardinitandtrueinreleaseInitalign with the conditional logic ininitPoolandinitPoolCollateral, enabling separate test and release configurations.Also applies to: 100-103, 189-189, 203-205
366-377: LGTM: protocolFee query function correctly added.The new
protocolFeefunction follows the existing pattern for query functions (usingsimulateTransactionandparseScvToJs) and correctly queries the pool'sprotocol_feemethod.
480-495: LGTM: liquidate signature correctly simplified.The removal of the
receiveStokenparameter aligns with the updated contract API (liquidate(env: Env, liquidator: Address, who: Address)incontracts/pool/src/lib.rs:163), simplifying the liquidation interface.
554-554: LGTM: WASM target path updated for Protocol 23.The path change from
wasm32-unknown-unknowntowasm32v1-nonealigns with Soroban Protocol 23's updated WASM target.
576-606: LGTM: liquidateCli signature correctly simplified.The removal of the
receiveStokenparameter fromliquidateCliis consistent with the updatedliquidatefunction and contract API.
702-702: LGTM: Budget snapshot API updated for Protocol 23.The changes from
.costto["cost"]and.readBytes()to.diskReadBytes()reflect updated Soroban RPC API in Protocol 23. The bracket notation suggests a schema change in the simulation response.Also applies to: 707-707
integration-tests/tests/pool/2.liquidator.spec.ts (3)
112-113: LGTM: Test scenario borrow amount adjusted.The borrow amount change to
89_999_000nis a test scenario adjustment, with corresponding assertion updates in lines 121-130.
121-130: LGTM: Assertions correctly updated for new borrow amount.All balance and position assertions (XLM balance, debt balance, sXLM balance, dXLM supply, debt/npv ranges) are mathematically consistent with the updated borrow amount of
89_999_000n.
178-178: LGTM: liquidate calls correctly updated to match new signature.The removal of the
falseparameter (formerlyreceiveStoken) aligns with the simplifiedliquidatefunction signature inpool.sut.ts:480-495.Also applies to: 229-229
| { | ||
| asset: "XLM", | ||
| asset_decimals: +process.env['XLM_DECIMALS'] ?? 7, | ||
| asset_decimals: +(process.env['XLM_DECIMALS'] ?? 7), |
There was a problem hiding this comment.
Verify fallback decimal values for XRP and USDC.
All three assets default to 7 decimals when environment variables are missing. However, XRP and USDC typically use 9 decimals (lines 80-81 in test init confirm this). The fallback ?? 7 may cause incorrect decimal handling in release mode if XRP_DECIMALS or USDC_DECIMALS are not set.
Apply this diff to correct the fallbacks:
asset: "XRP",
- asset_decimals: +(process.env['XRP_DECIMALS'] ?? 7),
+ asset_decimals: +(process.env['XRP_DECIMALS'] ?? 9),
max_sanity_price_in_base: BigInt(+process.env['XRP_MAX_SANITY_PRICE_IN_BASE']), asset: "USDC",
- asset_decimals: +(process.env['USDC_DECIMALS'] ?? 7),
+ asset_decimals: +(process.env['USDC_DECIMALS'] ?? 9),
max_sanity_price_in_base: BigInt(+process.env['USDC_MAX_SANITY_PRICE_IN_BASE']),Also applies to: 229-229, 244-244
🤖 Prompt for AI Agents
In integration-tests/tests/pool.sut.ts around lines 214, 229, and 244, the
decimal fallbacks incorrectly default XRP and USDC to 7; update the three places
so each asset uses the correct fallback: XLM should default to 7 and both XRP
and USDC should default to 9, and coerce the env value to a number (e.g.,
Number(process.env['XRP_DECIMALS'] ?? 9)) to ensure numeric decimals are used in
tests.
| async function initPool( | ||
| client: SorobanClient, | ||
| salt: string | ||
| salt: string, | ||
| isRelease: boolean | ||
| ): Promise<void> { | ||
| await initContract<Array<any>>( | ||
| "POOL", | ||
| () => | ||
| client.sendTransaction( | ||
| process.env.SLENDER_DEPLOYER, | ||
| "deploy_pool", | ||
| adminKeys, | ||
| 3, | ||
| convertToScvBytes(salt, "hex"), | ||
| convertToScvBytes(process.env.SLENDER_POOL_HASH, "hex"), | ||
| convertToScvAddress(adminKeys.publicKey()), | ||
| convertToScvMap({ | ||
| base_asset_address: convertToScvAddress(process.env[`SLENDER_TOKEN_${process.env[`BASE_ASSET`] ?? 'XLM'}`]), | ||
| base_asset_decimals: convertToScvU32(+process.env['BASE_ASSET_DECIMALS'] ?? 7), | ||
| flash_loan_fee: convertToScvU32(+process.env['FLASH_LOAN_FEE_BPS'] ?? 5), | ||
| grace_period: convertToScvU64(+process.env['GRACE_PERIOD_SEC'] ?? 1), | ||
| initial_health: convertToScvU32(+process.env['INITIAL_HEALTH_BPS'] ?? 2_500), | ||
| ir_alpha: convertToScvU32(+process.env['IR_ALPHA'] ?? 143), | ||
| ir_initial_rate: convertToScvU32(+process.env['IR_INITIAL_RATE_BPS'] ?? 200), | ||
| ir_max_rate: convertToScvU32(+process.env['IR_MAX_RATE_BPS'] ?? 50_000), | ||
| ir_scaling_coeff: convertToScvU32(+process.env['IR_SCALING_COEFF_BPS'] ?? 9_000), | ||
| liquidation_protocol_fee: convertToScvU32(+process.env['LIQUIDATION_PROTOCOL_FEE_BPS'] ?? 0), | ||
| min_collat_amount: convertToScvI128(process.env['MIN_COLLAT_AMOUNT_IN_BASE'] ? BigInt(process.env['MIN_COLLAT_AMOUNT_IN_BASE']) : 1n), | ||
| min_debt_amount: convertToScvI128(process.env['MIN_DEBT_AMOUNT_IN_BASE'] ? BigInt(process.env['MIN_DEBT_AMOUNT_IN_BASE']) : 1n), | ||
| timestamp_window: convertToScvU64(+process.env['TIMESTAMP_WINDOW_SEC'] ?? 20), | ||
| user_assets_limit: convertToScvU32(+process.env['USER_ASSET_LIMIT'] ?? 4), | ||
| }) | ||
| ), | ||
| () => { | ||
| if (isRelease) { | ||
| return client.sendTransaction( | ||
| process.env.SLENDER_DEPLOYER, | ||
| "deploy_pool", | ||
| adminKeys, | ||
| 3, | ||
| convertToScvBytes(salt, "hex"), | ||
| convertToScvBytes(process.env.SLENDER_POOL_HASH, "hex"), | ||
| convertToScvAddress(adminKeys.publicKey()), | ||
| convertToScvMap({ | ||
| base_asset_address: convertToScvAddress(process.env[`SLENDER_TOKEN_${process.env[`BASE_ASSET`]}`]), | ||
| base_asset_decimals: convertToScvU32(+(process.env['BASE_ASSET_DECIMALS'])), | ||
| flash_loan_fee: convertToScvU32(+(process.env['FLASH_LOAN_FEE_BPS'])), | ||
| grace_period: convertToScvU64(+(process.env['GRACE_PERIOD_SEC'])), | ||
| initial_health: convertToScvU32(+(process.env['INITIAL_HEALTH_BPS'])), | ||
| ir_alpha: convertToScvU32(+(process.env['IR_ALPHA'])), | ||
| ir_initial_rate: convertToScvU32(+(process.env['IR_INITIAL_RATE_BPS'])), | ||
| ir_max_rate: convertToScvU32(+(process.env['IR_MAX_RATE_BPS'])), | ||
| ir_scaling_coeff: convertToScvU32(+(process.env['IR_SCALING_COEFF_BPS'])), | ||
| liquidation_protocol_fee: convertToScvU32(+(process.env['LIQUIDATION_PROTOCOL_FEE_BPS'])), | ||
| min_collat_amount: convertToScvI128(BigInt(process.env['MIN_COLLAT_AMOUNT_IN_BASE'])), | ||
| min_debt_amount: convertToScvI128(BigInt(process.env['MIN_DEBT_AMOUNT_IN_BASE'])), | ||
| timestamp_window: convertToScvU64(+(process.env['TIMESTAMP_WINDOW_SEC'])), | ||
| user_assets_limit: convertToScvU32(+(process.env['USER_ASSET_LIMIT'])), | ||
| }) | ||
| ); | ||
| } else { | ||
| return client.sendTransaction( | ||
| process.env.SLENDER_DEPLOYER, | ||
| "deploy_pool", | ||
| adminKeys, | ||
| 3, | ||
| convertToScvBytes(salt, "hex"), | ||
| convertToScvBytes(process.env.SLENDER_POOL_HASH, "hex"), | ||
| convertToScvAddress(adminKeys.publicKey()), | ||
| convertToScvMap({ | ||
| base_asset_address: convertToScvAddress(process.env['SLENDER_TOKEN_XLM']), | ||
| base_asset_decimals: convertToScvU32(7), | ||
| flash_loan_fee: convertToScvU32(5), | ||
| grace_period: convertToScvU64(1), | ||
| initial_health: convertToScvU32(2_500), | ||
| ir_alpha: convertToScvU32(143), | ||
| ir_initial_rate: convertToScvU32(200), | ||
| ir_max_rate: convertToScvU32(50_000), | ||
| ir_scaling_coeff: convertToScvU32(9_000), | ||
| liquidation_protocol_fee: convertToScvU32(100), | ||
| min_collat_amount: convertToScvI128(1n), | ||
| min_debt_amount: convertToScvI128(1n), | ||
| timestamp_window: convertToScvU64(20), | ||
| user_assets_limit: convertToScvU32(4), | ||
| }) | ||
| ); | ||
| } | ||
| }, | ||
| (result) => result[0] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Validate required environment variables in release mode.
The isRelease=true branch reads 14+ environment variables (e.g., BASE_ASSET, BASE_ASSET_DECIMALS, FLASH_LOAN_FEE_BPS) without validation. Missing or invalid values will cause runtime failures. Consider adding validation or defaulting to test values if env vars are missing.
Run the following script to check which env vars are used but not documented:
🏁 Script executed:
#!/bin/bash
# Extract env var names from initPool and initPoolCollateral release branches
rg -nP "process\.env\[[\'\"]([A-Z_]+)[\'\"]" integration-tests/tests/pool.sut.ts | \
sed -E "s/.*process\.env\[[\'\"]([A-Z_]+)['\"].*/\1/" | \
sort -u | \
while read var; do
echo "Checking $var"
rg -q "^${var}=" .env.example 2>/dev/null || echo " ⚠️ $var not in .env.example"
doneLength of output: 3074
Document and validate environment variables used in the release branch.
The isRelease=true branch references numerous env vars absent from .env.example (e.g. BASE_ASSET_DECIMALS, FLASH_LOAN_FEE_BPS, GRACE_PERIOD_SEC, etc.); update .env.example to include all of them and add runtime checks to fail fast on undefined or invalid values.
🤖 Prompt for AI Agents
integration-tests/tests/pool.sut.ts lines 838-902: the release branch uses many
environment variables that are missing from .env.example and are not validated
at runtime; add all referenced vars to .env.example (BASE_ASSET,
BASE_ASSET_DECIMALS, FLASH_LOAN_FEE_BPS, GRACE_PERIOD_SEC, INITIAL_HEALTH_BPS,
IR_ALPHA, IR_INITIAL_RATE_BPS, IR_MAX_RATE_BPS, IR_SCALING_COEFF_BPS,
LIQUIDATION_PROTOCOL_FEE_BPS, MIN_COLLAT_AMOUNT_IN_BASE,
MIN_DEBT_AMOUNT_IN_BASE, TIMESTAMP_WINDOW_SEC, USER_ASSET_LIMIT,
SLENDER_TOKEN_<BASE_ASSET>, SLENDER_POOL_HASH, SLENDER_DEPLOYER) and implement a
fast-fail validation helper that runs before initPool (or at function start)
which checks each required env var is defined, parses numbers/BigInt safely (use
Number(), parseInt, BigInt where appropriate), validates ranges/types and throws
a clear error if missing or invalid so deployment fails fast with actionable
messages.
| async function initPoolCollateral( | ||
| client: SorobanClient, | ||
| asset: SlenderAsset, | ||
| order: number | ||
| order: number, | ||
| isRelease: boolean | ||
| ): Promise<void> { | ||
| await initContract(`POOL_${asset}_COLLATERAL_CONFIGURED`, () => | ||
| client.sendTransaction( | ||
| process.env.SLENDER_POOL, | ||
| "configure_as_collateral", | ||
| adminKeys, | ||
| 3, | ||
| convertToScvAddress(process.env[`SLENDER_TOKEN_${asset}`]), | ||
| convertToScvMap({ | ||
| // todo: trim to short string | ||
| discount: convertToScvU32(+process.env[`${asset}_DISCOUNT_BPS`] ?? 6000), | ||
| liq_cap: convertToScvI128(process.env[`${asset}_LIQUIDITY_CAP`] ? BigInt(process.env[`${asset}_LIQUIDITY_CAP`]) : 1000000000000000n), | ||
| pen_order: convertToScvU32(+process.env[`${asset}_PENALTY_ORDER`] ?? order), | ||
| util_cap: convertToScvU32(+process.env[`${asset}_UTILIZATION_CAP`] ?? 9000), | ||
| }) | ||
| ) | ||
| await initContract(`POOL_${asset}_COLLATERAL_CONFIGURED`, () => { | ||
| if (isRelease) { | ||
| return client.sendTransaction( | ||
| process.env.SLENDER_POOL, | ||
| "configure_as_collateral", | ||
| adminKeys, | ||
| 3, | ||
| convertToScvAddress(process.env[`SLENDER_TOKEN_${asset}`]), | ||
| convertToScvMap({ | ||
| // todo: trim to short string | ||
| discount: convertToScvU32(+(process.env[`${asset}_DISCOUNT_BPS`])), | ||
| liq_cap: convertToScvI128(BigInt(process.env[`${asset}_LIQUIDITY_CAP`])), | ||
| pen_order: convertToScvU32(+(process.env[`${asset}_PENALTY_ORDER`])), | ||
| util_cap: convertToScvU32(+(process.env[`${asset}_UTILIZATION_CAP`])), | ||
| }) | ||
| ); | ||
| } else { | ||
|
|
||
| return client.sendTransaction( | ||
| process.env.SLENDER_POOL, | ||
| "configure_as_collateral", | ||
| adminKeys, | ||
| 3, | ||
| convertToScvAddress(process.env[`SLENDER_TOKEN_${asset}`]), | ||
| convertToScvMap({ | ||
| // todo: trim to short string | ||
| discount: convertToScvU32(6000), | ||
| liq_cap: convertToScvI128(1000000000000000n), | ||
| pen_order: convertToScvU32(order), | ||
| util_cap: convertToScvU32(9000), | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Validate required environment variables for collateral config in release mode.
Similar to initPool, the isRelease=true branch reads asset-specific environment variables (${asset}_DISCOUNT_BPS, ${asset}_LIQUIDITY_CAP, etc.) without validation. Missing values will cause runtime failures.
Run the following script to check which asset-specific env vars are required:
🏁 Script executed:
#!/bin/bash
# Check which assets and their env vars are used
for asset in XLM XRP USDC RWA; do
echo "=== $asset ==="
for suffix in DISCOUNT_BPS LIQUIDITY_CAP PENALTY_ORDER UTILIZATION_CAP; do
var="${asset}_${suffix}"
rg -q "^${var}=" .env.example 2>/dev/null || echo " ⚠️ $var not in .env.example"
done
doneLength of output: 760
Add missing env vars to .env.example and validate them in initPoolCollateral.
- Update
.env.exampleto include, for each asset (XLM, XRP, USDC, RWA), the four variables:${asset}_DISCOUNT_BPS${asset}_LIQUIDITY_CAP${asset}_PENALTY_ORDER${asset}_UTILIZATION_CAP
- In the
isReleasebranch ofinitPoolCollateral(in integration-tests/tests/pool.sut.ts), assert that eachprocess.env[...]is defined and throw a clear error if any are missing before callingsendTransaction.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/pool/3.liquidator.spec.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration-tests/tests/pool/3.liquidator.spec.ts (2)
integration-tests/tests/pool.sut.ts (4)
liquidate(480-495)sTokenBalanceOf(280-292)accountPosition(322-333)withdraw(460-478)contracts/pool/src/methods/liquidate.rs (1)
liquidate(26-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (6)
integration-tests/tests/pool/3.liquidator.spec.ts (6)
193-193: Liquidate API correctly updated.The removal of the boolean flag parameter aligns with the updated function signature in pool.sut.ts.
266-266: Liquidate API correctly updated.Aligns with the updated function signature.
287-299: Assertions correctly reflect partial liquidation behavior.The updated assertions properly validate:
- USDC collateral remains untouched (line 287)
- Liquidator receives underlying XRP, not sTokens (lines 289-290)
- sXRP supply decreases as collateral is liquidated (line 293)
- Borrower's position remains solvent after partial liquidation (line 299)
The logic aligns with the partial liquidation flow described in the contract code.
302-308: Test correctly validates liquidation rejection.The test properly checks that liquidation is rejected when the borrower has no collateral remaining. The updated API call aligns with the new signature.
212-221: Verify sXrpSupply calculation logic.
Ensure40000000000n - (borrower1SXrpBalanceBefore - borrower1SXrpBalanceAfter)correctly reflects the post‐liquidation sXRP supply, including any protocol fees or rounding adjustments.
234-240: Ensure zero-amount withdrawal still rejects when position is unhealthy.I couldn’t locate the
withdrawimplementation to confirm its handling of a 0n amount; please verify that attempting to withdraw 0n when NPV ratio < 0.25 correctly triggers a rejection.
|
|
||
| // TODO: requires optimization | ||
| it("Case 6: Liquidator liquidates Borrower's positions partialy", async function () { | ||
| it("Case 5: Liquidator liquidates Borrower's positions partialy", async function () { |
There was a problem hiding this comment.
Fix typo in test description.
"partialy" should be "partially".
Apply this diff:
- it("Case 5: Liquidator liquidates Borrower's positions partialy", async function () {
+ it("Case 5: Liquidator liquidates Borrower's positions partially", async function () {📝 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("Case 5: Liquidator liquidates Borrower's positions partialy", async function () { | |
| it("Case 5: Liquidator liquidates Borrower's positions partially", async function () { |
🤖 Prompt for AI Agents
integration-tests/tests/pool/3.liquidator.spec.ts around line 183: the test
description contains a typo "partialy"; update the string to "partially" so the
it(...) reads: it("Case 5: Liquidator liquidates Borrower's positions
partially", async function () {.
| }); | ||
|
|
||
| it("Case 7: Borrower withdraw XRP partialy to NPV ~= 0", async function () { | ||
| it("Case 6: Borrower withdraw XRP partialy to NPV ~= 0", async function () { |
There was a problem hiding this comment.
Fix typo in test description.
"partialy" should be "partially".
Apply this diff:
- it("Case 6: Borrower withdraw XRP partialy to NPV ~= 0", async function () {
+ it("Case 6: Borrower withdraw XRP partially to NPV ~= 0", async function () {📝 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("Case 6: Borrower withdraw XRP partialy to NPV ~= 0", async function () { | |
| it("Case 6: Borrower withdraw XRP partially to NPV ~= 0", async function () { |
🤖 Prompt for AI Agents
integration-tests/tests/pool/3.liquidator.spec.ts around line 231: the test
description string contains a typo "partialy"; update the it(...) description to
use the correct spelling "partially" so the test reads: it("Case 6: Borrower
withdraw XRP partially to NPV ~= 0", async function () { ... });
| }); | ||
|
|
||
| it("Case 9: Liquidator liquidates Borrower's position partialy 2", async function () { | ||
| it("Case 8: Liquidator liquidates Borrower's position partialy 2", async function () { |
There was a problem hiding this comment.
Fix typo in test description.
"partialy" should be "partially".
Apply this diff:
- it("Case 8: Liquidator liquidates Borrower's position partialy 2", async function () {
+ it("Case 8: Liquidator liquidates Borrower's position partially 2", async function () {📝 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("Case 8: Liquidator liquidates Borrower's position partialy 2", async function () { | |
| it("Case 8: Liquidator liquidates Borrower's position partially 2", async function () { |
🤖 Prompt for AI Agents
In integration-tests/tests/pool/3.liquidator.spec.ts around line 252, the test
description contains a typo: change the string "Case 8: Liquidator liquidates
Borrower's position partialy 2" to "Case 8: Liquidator liquidates Borrower's
position partially 2" by editing the it(...) title accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/pool/4.npv.spec.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration-tests/tests/pool/4.npv.spec.ts (2)
integration-tests/tests/pool.sut.ts (9)
withdraw(460-478)tokenBalanceOf(308-320)sTokenBalanceOf(280-292)sTokenUnderlyingBalanceOf(352-364)sTokenTotalSupply(379-389)accountPosition(322-333)initPrice(1021-1043)healthFactor(62-64)deposit(422-439)contracts/pool/src/methods/withdraw.rs (1)
withdraw(28-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (7)
integration-tests/tests/pool/4.npv.spec.ts (7)
137-154: Test recalibration for Protocol 23 looks correct.The withdrawal amount reduction from 5 XRP to 0.0001 XRP and corresponding balance/NPV/collateral assertions are internally consistent. The smaller withdrawal correctly results in higher remaining balances (19,999,900,000n sXRP vs the previous ~15,000,000,000n) and slightly lower discounted collateral (119,999,400n).
185-187: Collateral assertion correctly reflects the lower XRP price.The updated discounted_collateral value (83_999_580n) appropriately reflects the XRP price drop to 0.7, resulting in lower collateral value and maintaining NPV < 0 as intended for this test case.
200-209: Balance and collateral assertions are consistent.The balance assertions correctly match the ending state from Case 4, and the discounted_collateral (83_999_580n) reflects the repriced collateral. Both properly validate that the withdrawal attempt is rejected without state changes.
225-237: Balance and position assertions correctly reflect the deposit.The assertions properly validate the 3.5 XRP deposit:
- sXRP balances increased by 3,500,000,000n (from 19,999,900,000n to 23,499,900,000n)
- XRP balance decreased correspondingly (76,500,100,000n)
- discounted_collateral increased to 98,699_580n reflecting additional collateral at the 0.7 price
- NPV improved to the 9.6M-10M range while maintaining health < 0.25
258-261: Position assertions correctly unchanged after rejected borrow.The discounted_collateral and NPV values properly match Case 8's ending state, confirming that the rejected borrow attempt did not modify the borrower's position.
265-289: Deposit amount increase and corresponding assertions are correct.The doubled deposit amount (from 3.5 XRP to 7 XRP) and all corresponding assertions are internally consistent:
- sXRP balances increased by 7,000,000,000n (from 23,499,900,000n to 30,499,900,000n)
- XRP balance decreased correspondingly (69,500,100,000n)
- discounted_collateral increased to 128,099,580n reflecting the substantial additional collateral
- NPV improved to the 39M-40M range, achieving the target health >= 0.25
310-313: Position assertions are acceptable given the ranges.The collateral correctly remains unchanged at 128,099,580n after the borrow operation. The NPV range (39M-40M) overlaps with Case 10 because the 90,000 debt increase is small relative to the 1M assertion range, which is acceptable for this integration test.
| // XRP price is set to 0.9 | ||
| await initPrice(client, "XRP", 9_000_000_000_000_000n, 0); | ||
| await initPrice(client, "XRP", 7_000_000_000_000_000n, 0); |
There was a problem hiding this comment.
Update comment to reflect actual price.
The comment states "XRP price is set to 0.9" but the code sets it to 7_000_000_000_000_000n, which represents 0.7 (not 0.9) assuming the standard precision.
Apply this diff to fix the comment:
- // XRP price is set to 0.9
+ // XRP price is set to 0.7
await initPrice(client, "XRP", 7_000_000_000_000_000n, 0);📝 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.
| // XRP price is set to 0.9 | |
| await initPrice(client, "XRP", 9_000_000_000_000_000n, 0); | |
| await initPrice(client, "XRP", 7_000_000_000_000_000n, 0); | |
| // XRP price is set to 0.7 | |
| await initPrice(client, "XRP", 7_000_000_000_000_000n, 0); |
🤖 Prompt for AI Agents
In integration-tests/tests/pool/4.npv.spec.ts around lines 158 to 159, the
inline comment incorrectly states "XRP price is set to 0.9" while the call
initPrice(client, "XRP", 7_000_000_000_000_000n, 0) sets the price to 0.7 (given
the standard precision); update the comment to accurately read "XRP price is set
to 0.7" (or equivalent wording) so it matches the numeric value used.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration-tests/tests/pool/6.tx.budget.spec.ts (1)
140-147: LGTM! Liquidate signature updated correctly.The liquidate call has been properly updated to match the new function signature (removing the asset and boolean flag parameters), consistent with the Protocol 23 migration to typed contract events shown in the relevant code snippets.
Consider removing the commented-out old code (Lines 143-146) if it's no longer needed for reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/pool/6.tx.budget.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration-tests/tests/pool/6.tx.budget.spec.ts (1)
integration-tests/tests/pool.sut.ts (3)
deposit(422-439)borrow(403-420)liquidate(480-495)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
| // Borrower2 deposits 20_000_000_000 USDC, borrows 60_000_000 XLM, 4_000_000_000 XRP | ||
| await deposit(client, borrower2Keys, "USDC", 20_000_000_000n); | ||
| await borrow(client, borrower2Keys, "XLM", 60_000_000n); | ||
| await borrow(client, borrower2Keys, "XRP", 5_900_000_000n); | ||
| await borrow(client, borrower2Keys, "XRP", 2_000_000_000n); |
There was a problem hiding this comment.
Fix the comment to match the actual borrow amount.
The comment on Line 80 states that Borrower2 borrows 4_000_000_000 XRP, but the code on Line 83 shows 2_000_000_000n. Update the comment to reflect the correct value.
Apply this diff:
- // Borrower2 deposits 20_000_000_000 USDC, borrows 60_000_000 XLM, 4_000_000_000 XRP
+ // Borrower2 deposits 20_000_000_000 USDC, borrows 60_000_000 XLM, 2_000_000_000 XRP📝 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.
| // Borrower2 deposits 20_000_000_000 USDC, borrows 60_000_000 XLM, 4_000_000_000 XRP | |
| await deposit(client, borrower2Keys, "USDC", 20_000_000_000n); | |
| await borrow(client, borrower2Keys, "XLM", 60_000_000n); | |
| await borrow(client, borrower2Keys, "XRP", 5_900_000_000n); | |
| await borrow(client, borrower2Keys, "XRP", 2_000_000_000n); | |
| // Borrower2 deposits 20_000_000_000 USDC, borrows 60_000_000 XLM, 2_000_000_000 XRP | |
| await deposit(client, borrower2Keys, "USDC", 20_000_000_000n); | |
| await borrow(client, borrower2Keys, "XLM", 60_000_000n); | |
| await borrow(client, borrower2Keys, "XRP", 2_000_000_000n); |
🤖 Prompt for AI Agents
integration-tests/tests/pool/6.tx.budget.spec.ts around lines 80 to 83: the
inline comment says Borrower2 borrows 4_000_000_000 XRP but the actual call
borrows 2_000_000_000n; update the comment to state 2_000_000_000 XRP so it
matches the code (i.e., change "4_000_000_000 XRP" to "2_000_000_000 XRP").
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Documentation