Open
Conversation
… finalize_session
… refund correctness
📝 WalkthroughWalkthroughAdds admin-configurable platform fee (bps, capped at 2000) and optional treasury. finalize_session now computes gross expert pay, derives fee_amount, pays treasury (if set and fee>0), pays expert net, refunds user, and emits session_finalized with expert_net_pay and fee_amount. Changes
Sequence DiagramsequenceDiagram
actor Admin as Admin
participant Contract
participant Storage
participant Treasury
participant Expert
participant User
rect rgba(100,149,237,0.5)
note right of Admin: Fee Configuration Phase
Admin->>Contract: set_fee(env, new_fee_bps)
activate Contract
Contract->>Contract: require_auth(admin)
Contract->>Storage: set_fee_bps(fee_bps)
deactivate Contract
Admin->>Contract: set_treasury(env, treasury)
activate Contract
Contract->>Contract: require_auth(admin)
Contract->>Storage: set_treasury(treasury_address)
deactivate Contract
end
rect rgba(144,238,144,0.5)
note right of Admin: Session Finalization Phase
Admin->>Contract: finalize_session(booking_id, ...)
activate Contract
Contract->>Storage: get_fee_bps()
Contract->>Contract: compute gross_expert_pay = rate_per_second * actual_duration
Contract->>Contract: fee_amount = gross_expert_pay * fee_bps / 10000
Contract->>Contract: expert_net_pay = gross_expert_pay - fee_amount
Contract->>Storage: get_treasury()
alt treasury exists & fee_amount > 0
Contract->>Treasury: transfer(fee_amount)
end
Contract->>Expert: transfer(expert_net_pay)
Contract->>User: transfer(refund)
Contract->>Contract: emit session_finalized(booking_id, actual_duration, expert_net_pay, fee_amount)
deactivate Contract
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
contracts/payment-vault-contract/src/lib.rs (1)
42-48: Add doc comments for consistency with other entrypoints.Other admin functions like
pauseandunpausehave documentation comments. Consider adding similar comments for these new entrypoints to maintain consistency.📝 Suggested documentation
+ /// Set the platform fee in basis points (Admin-only) + /// Maximum allowed fee is 2000 BPS (20%) pub fn set_fee(env: Env, new_fee_bps: u32) -> Result<(), VaultError> { contract::set_fee(&env, new_fee_bps) } + /// Set the treasury address for receiving platform fees (Admin-only) pub fn set_treasury(env: Env, treasury: Address) -> Result<(), VaultError> { contract::set_treasury(&env, &treasury) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/lib.rs` around lines 42 - 48, Add consistent Rust doc comments above the public entrypoints set_fee and set_treasury to match the style used for pause/unpause: describe purpose, parameters (env, new_fee_bps or treasury), expected behavior, and returned Result<(), VaultError>. Update the docstrings for the functions named set_fee and set_treasury so tooling and generated docs show the same level of detail as other admin entrypoints.contracts/payment-vault-contract/src/error.rs (1)
16-16: Minor: Gap in error code numbering.The error codes skip from
ExpertRateNotSet = 9toFeeTooHigh = 11, leaving value10unused. This doesn't affect functionality but may cause confusion if error codes are used for debugging or monitoring.💡 Suggested fix
- FeeTooHigh = 11, + FeeTooHigh = 10,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/error.rs` at line 16, The enum error codes have a gap between ExpertRateNotSet = 9 and FeeTooHigh = 11; update the variant values so they are contiguous—either change FeeTooHigh to = 10 or insert a placeholder/reserved variant for = 10 to document the unused code; locate the enum with ExpertRateNotSet and FeeTooHigh in error.rs and make the numeric adjustment so error codes are sequential and self-explanatory.contracts/payment-vault-contract/src/test.rs (2)
741-780: Consider adding test for fee without treasury configured.The current tests always set both fee and treasury together. However, in
finalize_session, whenfee_amount > 0but no treasury is set, the fee is silently not transferred (lines 174-178 in contract.rs). This could lead to funds being "locked" in the contract.Consider adding a test that verifies behavior when:
- Fee is configured (e.g., 10%)
- Treasury is NOT set
- Session is finalized
This would help document the expected behavior and catch any unintended changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 741 - 780, Add a unit test that configures a non-zero fee via set_fee but does NOT call set_treasury, then creates a booking (use set_my_rate, book_session, mint tokens as in existing tests) and calls finalize_session; assert expected balances for user, expert, contract (and treasury remains zero) to document current behavior when fee_amount > 0 but treasury is None; reference helper functions used in other tests (create_token_contract, create_client, set_my_rate, book_session, finalize_session) and mirror the structure of test_finalize_with_10_percent_fee while omitting the call to set_treasury so the test fails if fee handling or funds locking changes.
700-720: Tests could be strengthened with value verification.The test only verifies that
try_set_feeandtry_set_treasuryreturnOk, but doesn't verify that the values were actually stored correctly. Consider adding getter functions or verifying the effect throughfinalize_sessionbehavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 700 - 720, The test test_set_fee_and_treasury only asserts that client.try_set_fee and client.try_set_treasury return Ok; add assertions that the values were actually stored by reading them back (e.g., call a view/getter such as client.get_fee() and client.get_treasury() or the contract's public query methods) and assert equality with 1000_u32 and the treasury Address; if no getters exist, add minimal view methods to the contract or verify via finalize_session/other behavior that depends on those values to assert they took effect. Ensure you reference the same client instance used in the test and keep the added assertions immediately after each try_set_* call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 174-178: The code currently deducts fee_amount but only transfers
it when storage::get_treasury(env) returns Some, causing tokens to remain in the
contract if treasury is None; update the payment flow in the function that
computes and applies fee (where fee_amount, contract_address and
token_client.transfer are used) to handle the None case: if
storage::get_treasury(env) is None then treat fee_amount as zero (i.e., do not
deduct from the expert payout and do not call token_client.transfer), otherwise
proceed with the transfer to the treasury; alternatively (if you prefer stricter
behavior) enforce in set_fee that a treasury must be configured before allowing
non-zero fees by validating in set_fee and returning an error when treasury is
missing.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/error.rs`:
- Line 16: The enum error codes have a gap between ExpertRateNotSet = 9 and
FeeTooHigh = 11; update the variant values so they are contiguous—either change
FeeTooHigh to = 10 or insert a placeholder/reserved variant for = 10 to document
the unused code; locate the enum with ExpertRateNotSet and FeeTooHigh in
error.rs and make the numeric adjustment so error codes are sequential and
self-explanatory.
In `@contracts/payment-vault-contract/src/lib.rs`:
- Around line 42-48: Add consistent Rust doc comments above the public
entrypoints set_fee and set_treasury to match the style used for pause/unpause:
describe purpose, parameters (env, new_fee_bps or treasury), expected behavior,
and returned Result<(), VaultError>. Update the docstrings for the functions
named set_fee and set_treasury so tooling and generated docs show the same level
of detail as other admin entrypoints.
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 741-780: Add a unit test that configures a non-zero fee via
set_fee but does NOT call set_treasury, then creates a booking (use set_my_rate,
book_session, mint tokens as in existing tests) and calls finalize_session;
assert expected balances for user, expert, contract (and treasury remains zero)
to document current behavior when fee_amount > 0 but treasury is None; reference
helper functions used in other tests (create_token_contract, create_client,
set_my_rate, book_session, finalize_session) and mirror the structure of
test_finalize_with_10_percent_fee while omitting the call to set_treasury so the
test fails if fee handling or funds locking changes.
- Around line 700-720: The test test_set_fee_and_treasury only asserts that
client.try_set_fee and client.try_set_treasury return Ok; add assertions that
the values were actually stored by reading them back (e.g., call a view/getter
such as client.get_fee() and client.get_treasury() or the contract's public
query methods) and assert equality with 1000_u32 and the treasury Address; if no
getters exist, add minimal view methods to the contract or verify via
finalize_session/other behavior that depends on those values to assert they took
effect. Ensure you reference the same client instance used in the test and keep
the added assertions immediately after each try_set_* call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1c96142-aa9e-4fa9-960d-0ebe3b318a05
📒 Files selected for processing (6)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/events.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
Comment on lines
+174
to
+178
| if fee_amount > 0 { | ||
| if let Some(treasury) = storage::get_treasury(env) { | ||
| token_client.transfer(&contract_address, &treasury, &fee_amount); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential issue: Fee amount may be silently retained in contract when treasury is not set.
When fee_amount > 0 but get_treasury() returns None, the fee is deducted from the expert's pay but never transferred out. These tokens will remain locked in the contract with no mechanism to retrieve them.
Consider one of these approaches:
- Skip fee deduction if treasury is not set (treat as 0% fee)
- Require treasury to be set before allowing non-zero fee
- Fail the transaction if fee would be collected but treasury is missing
🔧 Option 1: Skip fee if no treasury (recommended)
let fee_bps = storage::get_fee_bps(env);
- let fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000;
- let expert_net_pay = gross_expert_pay - fee_amount;
+ let treasury_opt = storage::get_treasury(env);
+ let fee_amount = if treasury_opt.is_some() {
+ (gross_expert_pay * fee_bps as i128) / 10_000
+ } else {
+ 0
+ };
+ let expert_net_pay = gross_expert_pay - fee_amount;
// ...
if fee_amount > 0 {
- if let Some(treasury) = storage::get_treasury(env) {
+ if let Some(treasury) = treasury_opt {
token_client.transfer(&contract_address, &treasury, &fee_amount);
}
}🔧 Option 2: Require treasury when fee is set
In set_fee, enforce that treasury must already be configured:
pub fn set_fee(env: &Env, new_fee_bps: u32) -> Result<(), VaultError> {
let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?;
admin.require_auth();
if new_fee_bps > 2000 {
return Err(VaultError::FeeTooHigh);
}
+ if new_fee_bps > 0 && storage::get_treasury(env).is_none() {
+ return Err(VaultError::TreasuryNotSet); // New error variant needed
+ }
storage::set_fee_bps(env, new_fee_bps);
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/contract.rs` around lines 174 - 178, The
code currently deducts fee_amount but only transfers it when
storage::get_treasury(env) returns Some, causing tokens to remain in the
contract if treasury is None; update the payment flow in the function that
computes and applies fee (where fee_amount, contract_address and
token_client.transfer are used) to handle the None case: if
storage::get_treasury(env) is None then treat fee_amount as zero (i.e., do not
deduct from the expert payout and do not call token_client.transfer), otherwise
proceed with the transfer to the treasury; alternatively (if you prefer stricter
behavior) enforce in set_fee that a treasury must be configured before allowing
non-zero fees by validating in set_fee and returning an error when treasury is
missing.
Collaborator
|
Nice work @clintjeff2 . Please resolve the merge conflicts |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/payment-vault-contract/src/lib.rs (1)
42-48: Add doc comments for new public entrypoints.
set_feeandset_treasuryare admin-facing configuration methods that affect payment distribution. Adding doc comments (similar to other methods likepause/unpause) would clarify their purpose, parameters, and constraints (e.g., fee cap at 2000 BPS).📝 Proposed documentation
+ /// Set the platform fee in basis points (Admin-only). + /// Fee is capped at 2000 BPS (20%). Fee is deducted from expert payout on finalization. pub fn set_fee(env: Env, new_fee_bps: u32) -> Result<(), VaultError> { contract::set_fee(&env, new_fee_bps) } + /// Set the treasury address to receive platform fees (Admin-only). + /// Must be configured before setting a non-zero fee to avoid locked funds. pub fn set_treasury(env: Env, treasury: Address) -> Result<(), VaultError> { contract::set_treasury(&env, &treasury) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/lib.rs` around lines 42 - 48, Add Rust doc comments to the new public entrypoints set_fee and set_treasury similar to existing docs for pause/unpause: describe that these are admin-only configuration methods, document parameters (new_fee_bps as basis points with max cap 2000 BPS and treasury as the destination Address), explain effects on payment distribution and any constraints or errors (e.g., returns VaultError if fee exceeds cap or caller unauthorized), and mention who can call them. Reference the public functions set_fee and set_treasury when adding the /// comments directly above each function definition.contracts/payment-vault-contract/src/contract.rs (1)
176-176: Unchecked multiplication inconsistent withbook_session.
book_session(lines 111-113) useschecked_mulwithok_or(VaultError::Overflow)?forrate_per_second * max_duration, but here infinalize_sessionthe same pattern uses direct multiplication. While overflow is unlikely for realistic values (i128 can hold ~1.7e38, and typical products are much smaller), the inconsistency could mask edge-case bugs.♻️ Proposed fix for consistency
// 4. Calculate payments - let gross_expert_pay = booking.rate_per_second * (actual_duration as i128); + let gross_expert_pay = booking.rate_per_second + .checked_mul(actual_duration as i128) + .ok_or(VaultError::Overflow)?; let refund = booking.total_deposit - gross_expert_pay;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/contract.rs` at line 176, In finalize_session, the multiplication that computes gross_expert_pay (booking.rate_per_second * (actual_duration as i128)) should use checked_mul and propagate an Overflow error to match book_session; replace the direct multiplication in finalize_session with booking.rate_per_second.checked_mul(actual_duration as i128).ok_or(VaultError::Overflow)? (referencing booking.rate_per_second, actual_duration, the finalize_session function and the book_session pattern that uses checked_mul).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/payment-vault-contract/src/lib.rs`:
- Around line 50-52: The doc comment "/// Set an expert's own rate per second"
is placed above transfer_admin but belongs to set_my_rate; move or remove the
misplaced comment: remove that line above the transfer_admin declaration
(function transfer_admin) and ensure the same comment is present directly above
the set_my_rate function (function set_my_rate) so documentation matches the
implementation.
In `@contracts/payment-vault-contract/src/test.rs`:
- Line 701: The tests call client.init(&admin, &token, &oracle) but the contract
init signature expects (env, admin, token, oracle, registry) so the client.init
calls must include the missing registry argument; update each affected test
(test_set_fee_and_treasury, test_fee_cap_at_2000_bps,
test_finalize_with_10_percent_fee, test_finalize_with_fee_and_partial_refund,
test_finalize_zero_fee_behaves_as_before) to call client.init(&admin, &token,
&oracle, ®istry) (i.e., pass the fourth address for registry) so the test
harness matches the contract init signature and compiles.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Line 176: In finalize_session, the multiplication that computes
gross_expert_pay (booking.rate_per_second * (actual_duration as i128)) should
use checked_mul and propagate an Overflow error to match book_session; replace
the direct multiplication in finalize_session with
booking.rate_per_second.checked_mul(actual_duration as
i128).ok_or(VaultError::Overflow)? (referencing booking.rate_per_second,
actual_duration, the finalize_session function and the book_session pattern that
uses checked_mul).
In `@contracts/payment-vault-contract/src/lib.rs`:
- Around line 42-48: Add Rust doc comments to the new public entrypoints set_fee
and set_treasury similar to existing docs for pause/unpause: describe that these
are admin-only configuration methods, document parameters (new_fee_bps as basis
points with max cap 2000 BPS and treasury as the destination Address), explain
effects on payment distribution and any constraints or errors (e.g., returns
VaultError if fee exceeds cap or caller unauthorized), and mention who can call
them. Reference the public functions set_fee and set_treasury when adding the
/// comments directly above each function definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65b7f404-cf5e-4df0-ac58-94c6bc9a2999
📒 Files selected for processing (6)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/events.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/payment-vault-contract/src/events.rs
- contracts/payment-vault-contract/src/storage.rs
- contracts/payment-vault-contract/src/error.rs
Comment on lines
50
to
52
| /// Set an expert's own rate per second | ||
| /// Transfer admin rights to a new address (Admin-only) | ||
| /// Old admin instantly loses all privileges |
There was a problem hiding this comment.
Misplaced doc comment.
The comment /// Set an expert's own rate per second is positioned before transfer_admin but describes set_my_rate (line 63-68). This appears to be a leftover from editing.
🧹 Proposed fix
- /// Set an expert's own rate per second
/// Transfer admin rights to a new address (Admin-only)
/// Old admin instantly loses all privileges
pub fn transfer_admin(env: Env, new_admin: Address) -> Result<(), VaultError> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Set an expert's own rate per second | |
| /// Transfer admin rights to a new address (Admin-only) | |
| /// Old admin instantly loses all privileges | |
| /// Transfer admin rights to a new address (Admin-only) | |
| /// Old admin instantly loses all privileges | |
| pub fn transfer_admin(env: Env, new_admin: Address) -> Result<(), VaultError> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/lib.rs` around lines 50 - 52, The doc
comment "/// Set an expert's own rate per second" is placed above transfer_admin
but belongs to set_my_rate; move or remove the misplaced comment: remove that
line above the transfer_admin declaration (function transfer_admin) and ensure
the same comment is present directly above the set_my_rate function (function
set_my_rate) so documentation matches the implementation.
Collaborator
|
CI fails and you deleted key rotation tests. Please resolve this and avoid affecting existing code unrelated to changes required |
Author
|
@Bosun-Josh121 review and merge please |
- Merge origin/feature/revenue-service-fee (main branch changes) - Fix duplicate VaultError discriminant: FeeTooHigh reassigned to 13 (was 11, conflicting with SessionAlreadyStarted) - Fix misplaced doc comment in lib.rs: remove '/// Set an expert\''s own rate per second' from before transfer_admin (it belongs before set_my_rate) - Fix Platform Fee tests (test_set_fee_and_treasury, test_fee_cap_at_2000_bps, test_finalize_with_10_percent_fee, test_finalize_with_fee_and_partial_refund, test_finalize_zero_fee_behaves_as_before): add missing registry argument to client.init calls - Add key rotation tests: test_transfer_admin_success, test_transfer_admin_requires_current_admin_auth, test_transfer_admin_old_admin_loses_privileges, test_set_oracle_success, test_set_oracle_requires_admin_auth, test_set_oracle_old_oracle_loses_finalize_auth All 71 tests pass (25 identity-registry + 46 payment-vault)
Author
@Bosun-Josh121 that was a reckless mistake, please review the new changes. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contracts/payment-vault-contract/src/contract.rs (1)
249-251:⚠️ Potential issue | 🟠 MajorDon't deduct a fee unless
Treasuryis configured.
fee_amountis removed fromexpert_net_paybefore the code knows it can transfer to a treasury. With a non-zero fee and no treasury, the expert is underpaid and the delta stays trapped in the vault.Also applies to: 259-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/contract.rs` around lines 249 - 251, The code deducts fee_amount from expert_net_pay unconditionally; change it to only compute and subtract the fee when a Treasury is configured. Specifically, replace the unconditional use of storage::get_fee_bps/env fee math around fee_amount and expert_net_pay with a conditional: call the storage accessor that returns the configured Treasury (e.g., storage::get_treasury(env) or equivalent) and, if it returns Some(... ) and fee_bps > 0, compute fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000 and set expert_net_pay = gross_expert_pay - fee_amount; otherwise set expert_net_pay = gross_expert_pay and fee_amount = 0. Apply the same fix for the duplicated block referenced at lines 259-263.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 241-251: In finalize_session replace raw i128 math for
gross_expert_pay, refund, fee_amount and expert_net_pay with checked arithmetic
(checked_mul, checked_sub, checked_add, checked_div where appropriate) and
propagate any None to Err(VaultError::Overflow); specifically compute
gross_expert_pay using checked_mul, compute refund via checked_sub on
booking.total_deposit and gross_expert_pay, compute fee_amount by converting
fee_bps to i128 and using checked_mul followed by checked_div (or checked_mul
then checked_div with checks), then compute expert_net_pay via checked_sub, and
return Err(VaultError::Overflow) whenever a checked_* yields None instead of
allowing a panic.
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 980-983: The test currently only verifies failure with no auth by
calling env.set_auths(&[]) before client.try_finalize_session(&booking_id,
&50_u64); instead it must also assert that providing the old oracle's auth
(oracle_a) is rejected after rotation and that the new oracle's auth (oracle_b)
succeeds: set auths to oracle_a and assert try_finalize_session(&booking_id,
&50_u64) returns Err, then set auths to oracle_b and assert
try_finalize_session(...) returns Ok; use the existing env.set_auths,
try_finalize_session, booking_id, oracle_a and oracle_b identifiers to locate
and implement these checks.
- Around line 903-909: The test currently clears all auths (env.set_auths(&[]))
which causes client.try_pause() to fail regardless of who is stored as admin;
instead, explicitly authenticate as admin_a and assert try_pause() returns an
error to prove admin_a lost admin rights, then authenticate as admin_b and
assert try_pause() succeeds (is_ok) to prove rotation worked; update calls
around env.set_auths, using env.set_auths(&[admin_a]) to assert failure and
env.set_auths(&[admin_b]) to assert success, referencing client.try_pause(),
admin_a, and admin_b in the updated assertions.
---
Duplicate comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Around line 249-251: The code deducts fee_amount from expert_net_pay
unconditionally; change it to only compute and subtract the fee when a Treasury
is configured. Specifically, replace the unconditional use of
storage::get_fee_bps/env fee math around fee_amount and expert_net_pay with a
conditional: call the storage accessor that returns the configured Treasury
(e.g., storage::get_treasury(env) or equivalent) and, if it returns Some(... )
and fee_bps > 0, compute fee_amount = (gross_expert_pay * fee_bps as i128) /
10_000 and set expert_net_pay = gross_expert_pay - fee_amount; otherwise set
expert_net_pay = gross_expert_pay and fee_amount = 0. Apply the same fix for the
duplicated block referenced at lines 259-263.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b190363-021a-4118-8d54-10bce25571c9
📒 Files selected for processing (6)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/events.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
✅ Files skipped from review due to trivial changes (1)
- contracts/payment-vault-contract/src/events.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/payment-vault-contract/src/error.rs
Comment on lines
+241
to
+251
| // 4. Calculate payments | ||
| let gross_expert_pay = booking.rate_per_second * (actual_duration as i128); | ||
| let refund = booking.total_deposit - gross_expert_pay; | ||
|
|
||
| // Ensure calculations are valid | ||
| if expert_pay < 0 || refund < 0 { | ||
| if gross_expert_pay < 0 || refund < 0 { | ||
| return Err(VaultError::InvalidAmount); | ||
| } | ||
|
|
||
| let fee_bps = storage::get_fee_bps(env); | ||
| let fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000; | ||
| let expert_net_pay = gross_expert_pay - fee_amount; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== finalize_session settlement math ==="
sed -n '241,252p' contracts/payment-vault-contract/src/contract.rs
echo
echo "=== overflow-check settings in Cargo profiles ==="
fd -HI 'Cargo.toml|config.toml' . -t f | while read -r file; do
matches=$(rg -n '^\[profile\.release\]|overflow-checks' "$file" || true)
if [ -n "$matches" ]; then
echo "--- $file ---"
printf '%s\n' "$matches"
fi
doneRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1187
Use checked arithmetic for graceful error handling in finalize_session.
Lines 242–250 use raw i128 arithmetic. While the workspace enables overflow-checks = true in all Cargo profiles (which causes panics on overflow rather than silent wrapping), it's better to handle potential overflows explicitly and return VaultError::Overflow instead of crashing. This improves user experience and gives the contract control over error handling.
Proposed fix
// 4. Calculate payments
- let gross_expert_pay = booking.rate_per_second * (actual_duration as i128);
- let refund = booking.total_deposit - gross_expert_pay;
+ let gross_expert_pay = booking
+ .rate_per_second
+ .checked_mul(actual_duration as i128)
+ .ok_or(VaultError::Overflow)?;
+ let refund = booking
+ .total_deposit
+ .checked_sub(gross_expert_pay)
+ .ok_or(VaultError::Overflow)?;
- if gross_expert_pay < 0 || refund < 0 {
+ if refund < 0 {
return Err(VaultError::InvalidAmount);
}
let fee_bps = storage::get_fee_bps(env);
- let fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000;
- let expert_net_pay = gross_expert_pay - fee_amount;
+ let fee_amount = gross_expert_pay
+ .checked_mul(fee_bps as i128)
+ .ok_or(VaultError::Overflow)?
+ / 10_000;
+ let expert_net_pay = gross_expert_pay
+ .checked_sub(fee_amount)
+ .ok_or(VaultError::Overflow)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/contract.rs` around lines 241 - 251, In
finalize_session replace raw i128 math for gross_expert_pay, refund, fee_amount
and expert_net_pay with checked arithmetic (checked_mul, checked_sub,
checked_add, checked_div where appropriate) and propagate any None to
Err(VaultError::Overflow); specifically compute gross_expert_pay using
checked_mul, compute refund via checked_sub on booking.total_deposit and
gross_expert_pay, compute fee_amount by converting fee_bps to i128 and using
checked_mul followed by checked_div (or checked_mul then checked_div with
checks), then compute expert_net_pay via checked_sub, and return
Err(VaultError::Overflow) whenever a checked_* yields None instead of allowing a
panic.
Comment on lines
+903
to
+909
| // admin_a should no longer be able to call admin-only operations | ||
| // (only admin_b is now the admin; mock_all_auths allows both, | ||
| // so we disable and re-mock only admin_a to verify it fails) | ||
| env.set_auths(&[]); | ||
|
|
||
| // Without any valid auth for admin_b, pause should fail | ||
| // (admin_b is now the required auth, but no auth is mocked) | ||
| let result = client.try_pause(); | ||
| assert!(result.is_err()); | ||
| // Attempting a pause (admin-only) with no auth should fail | ||
| let res = client.try_pause(); | ||
| assert!(res.is_err()); |
There was a problem hiding this comment.
This test does not prove admin_a lost admin-only access.
Clearing all auths makes try_pause() fail regardless of which address is stored as admin. To validate the rotation, this should authenticate as admin_a and show failure, then authenticate as admin_b and show success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/test.rs` around lines 903 - 909, The
test currently clears all auths (env.set_auths(&[])) which causes
client.try_pause() to fail regardless of who is stored as admin; instead,
explicitly authenticate as admin_a and assert try_pause() returns an error to
prove admin_a lost admin rights, then authenticate as admin_b and assert
try_pause() succeeds (is_ok) to prove rotation worked; update calls around
env.set_auths, using env.set_auths(&[admin_a]) to assert failure and
env.set_auths(&[admin_b]) to assert success, referencing client.try_pause(),
admin_a, and admin_b in the updated assertions.
Comment on lines
+980
to
+983
| // Now finalize without any auth — should fail (oracle_a no longer valid) | ||
| env.set_auths(&[]); | ||
| let res = client.try_finalize_session(&booking_id, &50_u64); | ||
| assert!(res.is_err()); |
There was a problem hiding this comment.
This only checks “no auth”, not “old oracle rejected”.
env.set_auths(&[]) makes try_finalize_session() fail even if oracle_a is still accepted. The regression test should explicitly try finalizing with oracle_a auth after rotation, and with oracle_b auth as the success case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/test.rs` around lines 980 - 983, The
test currently only verifies failure with no auth by calling env.set_auths(&[])
before client.try_finalize_session(&booking_id, &50_u64); instead it must also
assert that providing the old oracle's auth (oracle_a) is rejected after
rotation and that the new oracle's auth (oracle_b) succeeds: set auths to
oracle_a and assert try_finalize_session(&booking_id, &50_u64) returns Err, then
set auths to oracle_b and assert try_finalize_session(...) returns Ok; use the
existing env.set_auths, try_finalize_session, booking_id, oracle_a and oracle_b
identifiers to locate and implement these checks.
Collaborator
|
Still errors in CI @clintjeff2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(vault): implement platform revenue service fee (#31)
Summary
Extracts a configurable percentage-based fee (in Basis Points, 10000 = 100%) from the expert's gross pay on every successfully finalized session, routing it to a designated treasury address. The user's refund is unaffected.
Proof of Successful Build & Tests
Changes
src/error.rsFeeTooHigh = 11— returned whenset_feeis called with a value above 2000 BPS (20%).src/storage.rsDataKey::FeeBpsandDataKey::Treasuryvariants to theDataKeyenum.set_fee_bps/get_fee_bps(instance storage, defaults to 0).set_treasury/get_treasury(instance storage).src/events.rssession_finalizedsignature to includefee_amount: i128as a third payload field, giving off-chain indexers full visibility into fee extraction.src/contract.rsset_fee(env, new_fee_bps)— Admin-only. Rejects values above 2000 BPS.set_treasury(env, treasury)— Admin-only. Stores the treasury address.finalize_session— Updated payment calculation:fee_amount → treasury,expert_net → expert,refund → user.src/lib.rspub fn set_fee(env: Env, new_fee_bps: u32) -> Result<(), VaultError>.pub fn set_treasury(env: Env, treasury: Address) -> Result<(), VaultError>.src/test.rstest_set_fee_and_treasury— verifies admin can set both values.test_fee_cap_at_2000_bps— 2000 BPS accepted, 2001 rejected.test_finalize_with_10_percent_fee— full session: treasury=100, expert=900, user unchanged.test_finalize_with_fee_and_partial_refund— partial session: treasury=50, expert=450, user refunded 500.test_finalize_zero_fee_behaves_as_before— no regression when fee is not set.Acceptance Criteria
fee = gross × bps / 10000).FeeTooHigh.Closes
Closes #31
Summary by CodeRabbit
New Features
Tests