Skip to content

refactor(soroban): use u128 across vault amount surfaces#420

Open
carrion256 wants to merge 3 commits into
devfrom
spr/refactor/vault-ergonomics/b0b2fa2d
Open

refactor(soroban): use u128 across vault amount surfaces#420
carrion256 wants to merge 3 commits into
devfrom
spr/refactor/vault-ergonomics/b0b2fa2d

Conversation

@carrion256

@carrion256 carrion256 commented Apr 23, 2026

Copy link
Copy Markdown
Collaborator

@coderabbitai

coderabbitai Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@carrion256, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 7 minutes and 12 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 03e559f1-0457-4167-ad4f-bcdc7e08914b

📥 Commits

Reviewing files that changed from the base of the PR and between 12c773d and 0e21a80.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (19)
  • Cargo.toml
  • contract/proxy-4626-soroban/src/contract.rs
  • contract/proxy-4626-soroban/src/tests.rs
  • contract/proxy-4626-soroban/tests/integration_tests.rs
  • contract/proxy-curator-soroban/Cargo.toml
  • contract/proxy-curator-soroban/src/contract.rs
  • contract/proxy-curator-soroban/src/error.rs
  • contract/proxy-curator-soroban/src/lib.rs
  • contract/proxy-curator-soroban/src/tests.rs
  • contract/proxy-curator-soroban/tests/integration_tests.rs
  • contract/vault/soroban/governance/src/lib.rs
  • contract/vault/soroban/governance/src/tests.rs
  • contract/vault/soroban/governance/src/types.rs
  • contract/vault/soroban/shared-types/src/lib.rs
  • contract/vault/soroban/src/contract/entrypoints.rs
  • contract/vault/soroban/src/contract/helpers.rs
  • contract/vault/soroban/src/tests.rs
  • contract/vault/soroban/tests/blend_e2e.rs
  • contract/vault/soroban/tests/integration_tests.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch spr/refactor/vault-ergonomics/b0b2fa2d

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

@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/c7101279 branch from e56db7e to 9c8572d Compare April 24, 2026 15:07
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from 7b6e2b9 to 29a7f67 Compare April 24, 2026 15:08
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/c7101279 branch from 9c8572d to 2da85b1 Compare April 28, 2026 12:06
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from 29a7f67 to f6add76 Compare April 28, 2026 12:06
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/c7101279 branch from 2da85b1 to dc019ba Compare April 28, 2026 12:08
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from f6add76 to add9ad4 Compare April 28, 2026 12:08
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/c7101279 branch from dc019ba to ab7f226 Compare April 29, 2026 13:25
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from add9ad4 to d4024b8 Compare April 29, 2026 13:30
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/c7101279 branch from ab7f226 to 9258b6b Compare April 30, 2026 15:11
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from d4024b8 to 4aefbe0 Compare April 30, 2026 15:11
@carrion256

Copy link
Copy Markdown
Collaborator Author

@coderabbitai for the benefit of the human reviewers - please output a description of this PR and any critique on the usability of the design when you are reviewing.

@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details and files changed
gh pr view 420 --repo Templar-Protocol/contracts --json title,body,files,additions,deletions,changedFiles 2>/dev/null || echo "Error fetching PR"

echo "---FILES---"
gh pr diff 420 --repo Templar-Protocol/contracts 2>/dev/null | head -600

Length of output: 24108


🏁 Script executed:

#!/bin/bash
gh pr diff 420 --repo Templar-Protocol/contracts 2>/dev/null | tail -n +601 | head -n 600

Length of output: 23485


🏁 Script executed:

#!/bin/bash
gh pr diff 420 --repo Templar-Protocol/contracts 2>/dev/null | tail -n +1201 | head -n 800

Length of output: 30318


🏁 Script executed:

#!/bin/bash
gh pr diff 420 --repo Templar-Protocol/contracts 2>/dev/null | tail -n +2001 | head -n 400

Length of output: 16230


@carrion256: Here is a summary and design critique for human reviewers.


PR #420refactor(soroban): use u128 across vault amount surfaces

What This PR Does

This is a type-system refactor: every amount-carrying surface in the Soroban vault runtime (deposits, withdrawals, shares, fees, caps, previews, allocations) is changed from i128 to u128. The change touches 15 files across:

  • contract/vault/soroban/src/ (entrypoints, helpers)
  • contract/vault/soroban/shared-types/ (wire codec)
  • contract/vault/soroban/governance/ (fee/cap submission)
  • contract/proxy-4626-soroban/ (ERC-4626 proxy)
  • contract/proxy-curator-soroban/ (curator proxy)
  • Integration and unit tests throughout

The net line count is −576 / +438 — deletions exceed additions because an entire layer of defensive runtime guards is removed by making the invariant impossible at compile time.

Specifically eliminated:

  • require_non_negative(i128) guards in the 4626 proxy (5 call sites)
  • to_wad(i128)? / to_optional_wad / cap_to_u128 / nonnegative_i128_to_u128 helpers in governance
  • require_non_negative_bounded_wad → replaced by require_bounded_wad(u128, max) (only upper-bound check survives)
  • optional_wad(Option<i128>) helper in helpers.rs
  • Multiple to_u128()/to_i128() conversion round-trips inside proxy_view and deposit_with_min_impl
  • The entire test_rejects_negative_amounts_before_vault_call and test_withdraw_rejects_negative_preview_shares_before_allowance test cases (now untestable by construction)

Wire codec layer (shared-types/src/lib.rs): push_i128/read_i128, push_option_i128/read_option_i128, and VaultCommandResult::I128 are all renamed to u128 variants. The raw little-endian byte layout is identical for any value that was non-negative before, so existing valid encoded payloads are preserved.

Bonus readability fix: proxy_view's return type was a deeply-nested anonymous 3-level tuple (26 lines of type annotation). It is now Result<ProxyViewResponse, ContractError>, using the shared-types alias.


Design Critique

✅ Strengths

  1. Invariant moved to the type system. All the "must not be negative" runtime checks are gone because u128 cannot represent negative values. This is the correct pattern and reduces the attack surface.

  2. ERC-4626 alignment. The EIP-4626 standard defines all amounts as uint256. Using u128 on the Soroban side is a cleaner semantic match than the previous i128.

  3. Thorough scope. The PR is internally consistent — all 15 files, all tests, all mock contracts, and the wire codec are updated together with no stragglers.

  4. validate_action simplification in governance. to_wad(value: i128)? (fallible, error path for negatives) becomes Wad::from(value: u128) (infallible). The error path only existed to guard against a type-system bypass; it is now unreachable by construction and is cleanly removed.


⚠️ Areas to Scrutinise

  1. Soroban token interface friction — boundary conversions are now manual.
    Soroban's native token standard uses i128 for balances and transfer amounts. The PR adds token_i128_to_u128() (in the 4626 proxy) and to_i128(amount)? (in allocate_impl) at these boundaries. The concern: a negative value from the Soroban token layer (e.g., a negative allowance or balance, which is technically possible on-chain) would now produce a ContractError::VaultError rather than any vault-specific error. This may be acceptable, but reviewers should confirm the error semantics are observable/debuggable for operators.

  2. max_deposit / max_mint now return values up to u128::MAX.
    Previously the values were clamped to i128::MAX (≈ 9.2 × 10¹⁸) to fit in i128. They now return u128::MAX.saturating_sub(state.total_assets) (≈ 3.4 × 10³⁸). The integration test is updated to assert u128::MAX - deposit_assets. External callers (e.g., off-chain aggregators or front-end integrations) that interpret max_deposit as a practical upper bound for UX purposes will now receive an astronomically large number. This is semantically correct per ERC-4626 ("maximum amount the vault currently accepts"), but worth confirming that no downstream client treats this as a meaningful deposit limit.

  3. Wire codec is a breaking change for any encoded negative i128 — but unversioned.
    VaultCommandResult::I128(value)VaultCommandResult::U128(value) at tag byte 1. The byte layout is identical for non-negative values, so all previously-valid wire payloads decode correctly. However, if any persisted test fixture or integration harness had encoded a negative i128 via the old codec, it would now decode as a very large u128. There is no version field in the codec to detect this. Given the greenfield learning (no live persisted state), this is safe — but reviewers should confirm no external harnesses (e.g., CI replay fixtures) hold serialized VaultCommand or GovernanceCommand payloads from before this change.

  4. vault_view_from_response return type change in proxy-curator.
    This function was previously infallible (-> VaultView); it now returns Result<VaultView, ContractError>. The change is forced by the cap_groups copy loop. The error path would only trigger if the iterator is malformed (which can't happen in practice given the types). Callers have been updated, but reviewers should confirm no existing callers expect the infallible version through a trait or dynamic dispatch path.

  5. Zero-amount rejection semantic preserved but worth verifying at the kernel boundary.
    if assets <= 0 (i128) → if assets == 0 (u128) in deposit_with_min_impl and request_withdraw_impl. Functionally equivalent (u128 can't be negative, and zero is still rejected). Reviewers should confirm the vault kernel also rejects zero-amount operations internally — if the kernel has its own > 0 check, this is belt-and-suspenders; if it does not, the entrypoint check is the only gate.


🧠 Learnings used
Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/curator-primitives/src/policy/cooldown/mod.rs:17-23
Timestamp: 2026-04-27T09:33:55.992Z
Learning: In Templar-Protocol/contracts, the Soroban vault (`contract/vault/soroban/`) and NEAR vault (`contract/vault/near/`) are greenfield deployments with no live persisted state. Do not flag internal serialization format changes (e.g., field type changes in structs like `Cooldown`, `RefreshThrottle`, or policy state) as breaking migration risks — there is no existing stored data to break.

Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: common/src/vault/mod.rs:0-0
Timestamp: 2026-04-27T12:49:18.945Z
Learning: In Templar-Protocol/contracts, `contract/vault/kernel` and `contract/vault/curator-primitives` crates intentionally import `Wad`, `Number`, `WIDE`, fee constants, and math helpers directly from `templar_vault_kernel` rather than through `templar_common::vault::wad`. The `templar_common::vault::prelude` re-exports this surface only for external consumers. Do not flag these internal kernel/curator imports as incomplete refactoring or suggest redirecting them through `templar_common`.

Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : Read README.md, STRIDE.md, SIZE_BUDGET.md, src/contract/entrypoints.rs, src/effects/mod.rs, and src/storage/mod.rs before making non-trivial changes to the Soroban vault runtime

Learnt from: peer2f00l
Repo: Templar-Protocol/contracts PR: 404
File: contract/vault/kernel/src/actions/mod.rs:1869-1871
Timestamp: 2026-04-14T07:35:09.869Z
Learning: In `contract/vault/kernel/src/actions/mod.rs` (Templar-Protocol/contracts, Rust), the vault kernel crate is compiled for Soroban (wasm32 target). Extracting helper functions from large dispatcher functions like `dispatch::apply_action` risks increasing wasm binary size due to additional call frames and reduced compiler inlining. Therefore, `#[allow(clippy::too_many_lines)]` suppressions on large dispatch/entrypoint functions in this file are intentional and should not be flagged as needing refactoring.

Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/** : When size gate fails, prefer deleting duplicated logic or narrowing compile surface before introducing new helper layers or architectural churn

Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/soroban/governance/src/types.rs:160-174
Timestamp: 2026-04-27T09:36:09.179Z
Learning: In `contract/vault/soroban/governance/src/types.rs` (Templar-Protocol/contracts), the `RestrictionMode` enum's `from_u32`/`as_u32` mapping uses `0=None, 1=Blacklist, 2=Whitelist`. The old `Paused` variant (previously mapped to `1`) has been removed entirely; pause state is now handled separately via `Gate.paused`. This is a greenfield Soroban governance ABI with no legacy clients/relayers, so recycling code `1` for `Blacklist` is intentional and safe. Do not flag this numeric remapping as a migration risk in future reviews.

Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : If behavior changes around governance bridging, withdrawal lifecycle, or share-token auth, update the docs in contract/vault/soroban/ in the same change

Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/effects/** : The kernel-to-Soroban address mapping is critical for effect routing. Treat changes there as high impact

Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/** : Avoid string formatting in release-critical runtime paths when static strings or compact error codes are sufficient

Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : Use `just -f contract/vault/soroban/justfile build` to build the Soroban runtime and `just -f contract/vault/soroban/justfile size-budget-check` to verify the 128 KiB size constraint

@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/c7101279 branch 7 times, most recently from 95e411b to 37bbf23 Compare June 8, 2026 08:48
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from 4aefbe0 to c10d2cf Compare June 8, 2026 09:23
@carrion256
carrion256 changed the base branch from spr/refactor/vault-ergonomics/c7101279 to dev June 8, 2026 09:23
@carrion256
carrion256 force-pushed the spr/refactor/vault-ergonomics/b0b2fa2d branch from c10d2cf to 0e21a80 Compare June 8, 2026 09:31
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.43590% with 410 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
contract/proxy-curator-soroban/src/contract.rs 46.67% 385 Missing ⚠️
contract/proxy-4626-soroban/src/contract.rs 56.25% 21 Missing ⚠️
contract/proxy-curator-soroban/src/error.rs 60.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant