feat: add expert rate support to payment vault#29
Merged
Bosun-Josh121 merged 2 commits intoLightForgeHub:mainfrom Feb 25, 2026
Merged
feat: add expert rate support to payment vault#29Bosun-Josh121 merged 2 commits intoLightForgeHub:mainfrom
Bosun-Josh121 merged 2 commits intoLightForgeHub:mainfrom
Conversation
Contributor
Author
|
@Bosun-Josh121 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces on-chain expert pricing: experts can call set_my_rate to store a per-second rate; book_session no longer accepts a rate and instead loads the expert's rate from storage, computes deposit, and fails if the expert rate is not set. Supporting error, event, storage, lib, and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Expert as Expert
participant User as User
participant Contract as Payment Vault Contract
participant Storage as Storage
participant Ledger as Ledger/Token
Expert->>Contract: set_my_rate(rate_per_second)
Contract->>Contract: require_auth & validate rate > 0
Contract->>Storage: set_expert_rate(expert, rate_per_second)
Storage-->>Contract: ack
Contract->>Contract: emit ExpertRateUpdated event
Contract-->>Expert: success
User->>Contract: book_session(user, expert, max_duration)
Contract->>Storage: get_expert_rate(expert)
Storage-->>Contract: rate_per_second (or none)
alt rate set
Contract->>Contract: compute total_deposit = rate * max_duration
Contract->>Ledger: transfer_from(user -> contract, total_deposit)
Ledger-->>Contract: transfer ack
Contract->>Contract: create booking record & emit BookingCreated
Contract-->>User: booking_id
else rate missing
Contract-->>User: error ExpertRateNotSet
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
dinahmaccodes
force-pushed
the
issue-23
branch
from
063eb26 to
567d5e4
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/payment-vault-contract/src/contract.rs (1)
58-58:⚠️ Potential issue | 🔴 CriticalUse checked arithmetic to avoid runtime traps.
Line [58], Line [116], Line [117], and Line [171] currently use unchecked math. Large inputs can overflow and panic, causing transaction failure instead of a controlled
VaultError.🛠️ Proposed fix (checked math + explicit error path)
@@ - let total_deposit = rate_per_second * (max_duration as i128); + let total_deposit = rate_per_second + .checked_mul(max_duration as i128) + .ok_or(VaultError::InvalidAmount)?; @@ - let expert_pay = booking.rate_per_second * (actual_duration as i128); - let refund = booking.total_deposit - expert_pay; + let expert_pay = booking + .rate_per_second + .checked_mul(actual_duration as i128) + .ok_or(VaultError::InvalidAmount)?; + let refund = booking + .total_deposit + .checked_sub(expert_pay) + .ok_or(VaultError::InvalidAmount)?; @@ - if current_time <= booking.created_at + RECLAIM_TIMEOUT { + let reclaim_after = booking + .created_at + .checked_add(RECLAIM_TIMEOUT) + .ok_or(VaultError::InvalidAmount)?; + if current_time <= reclaim_after { return Err(VaultError::ReclaimTooEarly); }Also applies to: 116-117, 171-171
🤖 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 58, Replace the unchecked multiplications with checked arithmetic and return a controlled VaultError on overflow: where you compute total_deposit using rate_per_second and max_duration (and the other occurrences at the sites using the same pattern), use checked_mul/checked_add/checked_sub as appropriate (e.g., rate_per_second.checked_mul(max_duration as i128)) and map None to Err(VaultError::ArithmeticOverflow) (or the existing numeric error variant) instead of letting the operation panic; update the expressions that set total_deposit and the calculations at the other referenced sites (lines using rate_per_second, max_duration, and any similar multiplications) to follow this pattern so all overflow paths return a VaultError.
🤖 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/test.rs`:
- Around line 61-64: Tests bind the result to _booking_id but later assertions
and calls use booking_id, causing undefined identifier errors; update each block
that currently declares _booking_id (e.g., the block calling
client.set_my_rate(&expert, &rate_per_second); client.book_session(&user,
&expert, &max_duration)) to bind to booking_id instead, and apply the same
rename in all other similar blocks (those calling client.book_session /
client.set_my_rate sequences) so downstream uses of booking_id resolve
correctly.
---
Outside diff comments:
In `@contracts/payment-vault-contract/src/contract.rs`:
- Line 58: Replace the unchecked multiplications with checked arithmetic and
return a controlled VaultError on overflow: where you compute total_deposit
using rate_per_second and max_duration (and the other occurrences at the sites
using the same pattern), use checked_mul/checked_add/checked_sub as appropriate
(e.g., rate_per_second.checked_mul(max_duration as i128)) and map None to
Err(VaultError::ArithmeticOverflow) (or the existing numeric error variant)
instead of letting the operation panic; update the expressions that set
total_deposit and the calculations at the other referenced sites (lines using
rate_per_second, max_duration, and any similar multiplications) to follow this
pattern so all overflow paths return a VaultError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 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
dinahmaccodes
force-pushed
the
issue-23
branch
from
567d5e4 to
d2d67f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/payment-vault-contract/src/test.rs (2)
273-276: Consider extracting a helper for repeated “set rate + book” setup.This pattern is repeated many times; a small helper will reduce drift and make test intent clearer.
♻️ Suggested refactor
+fn set_rate_and_book( + client: &PaymentVaultContractClient<'_>, + user: &Address, + expert: &Address, + rate_per_second: i128, + max_duration: u64, +) -> u32 { + client.set_my_rate(expert, &rate_per_second); + client.book_session(user, expert, &max_duration) +}- let booking_id = { - client.set_my_rate(&expert, &rate_per_second); - client.book_session(&user, &expert, &max_duration) - }; + let booking_id = set_rate_and_book(&client, &user, &expert, rate_per_second, max_duration);🤖 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 273 - 276, Extract the repeated "set rate + book" sequence into a small helper function (e.g., fn set_rate_and_book(client: &mut Client, user: &User, expert: &Expert, rate: &Rate, max_duration: &Duration) -> BookingId) that calls client.set_my_rate(&expert, &rate_per_second) then client.book_session(&user, &expert, &max_duration) and returns the booking id; replace the inline block that assigns booking_id with a call to set_rate_and_book to reduce duplication across tests and make intent clearer (update all occurrences of client.set_my_rate and client.book_session to use this new helper).
700-724: Rate update test does not yet prove updated value is actually used.At Line 714-723, only
is_ok/is_erris asserted. Add an observable assertion (e.g., book after update and verify deposit uses the new rate) to catch storage/write-path regressions.🤖 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 - 724, The test test_expert_can_set_and_update_rate currently only checks try_set_my_rate return values; extend it to assert the updated rate is actually stored/used by reading back the expert's rate (e.g., call a read method on the client like get_rate or query storage for the expert) or by performing an action that uses the rate (e.g., create a deposit/booking via the client and verify the computed fee reflects the new 25_i128 rate) after the update; locate and update uses of client.try_set_my_rate, client.init, and create_client to add the observable assertion that the storage/write-path applied the new rate.
🤖 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/test.rs`:
- Around line 780-783: Replace the broad assert!(res.is_err()) after calling
client.try_book_session(&user, &expert, &max_duration) with a precise assertion
that the returned error matches the ExpertRateNotSet variant (e.g.,
pattern-match the Result/Err to ensure it is ExpertRateNotSet) and additionally
verify that no balances changed for the involved parties (capture balances/state
for user and expert before and after the call and assert they are equal) so the
test ensures the specific rejection path and no side-effects.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 273-276: Extract the repeated "set rate + book" sequence into a
small helper function (e.g., fn set_rate_and_book(client: &mut Client, user:
&User, expert: &Expert, rate: &Rate, max_duration: &Duration) -> BookingId) that
calls client.set_my_rate(&expert, &rate_per_second) then
client.book_session(&user, &expert, &max_duration) and returns the booking id;
replace the inline block that assigns booking_id with a call to
set_rate_and_book to reduce duplication across tests and make intent clearer
(update all occurrences of client.set_my_rate and client.book_session to use
this new helper).
- Around line 700-724: The test test_expert_can_set_and_update_rate currently
only checks try_set_my_rate return values; extend it to assert the updated rate
is actually stored/used by reading back the expert's rate (e.g., call a read
method on the client like get_rate or query storage for the expert) or by
performing an action that uses the rate (e.g., create a deposit/booking via the
client and verify the computed fee reflects the new 25_i128 rate) after the
update; locate and update uses of client.try_set_my_rate, client.init, and
create_client to add the observable assertion that the storage/write-path
applied the new rate.
Comment on lines
+780
to
+783
| let res = client.try_book_session(&user, &expert, &max_duration); | ||
|
|
||
| assert!(res.is_err()); | ||
| } |
There was a problem hiding this comment.
The unset-rate failure assertion is too broad.
assert!(res.is_err()) can pass for unrelated failures. Assert the specific ExpertRateNotSet path (and ideally no balance movement) to lock this acceptance criterion.
🤖 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 780 - 783, Replace
the broad assert!(res.is_err()) after calling client.try_book_session(&user,
&expert, &max_duration) with a precise assertion that the returned error matches
the ExpertRateNotSet variant (e.g., pattern-match the Result/Err to ensure it is
ExpertRateNotSet) and additionally verify that no balances changed for the
involved parties (capture balances/state for user and expert before and after
the call and assert they are equal) so the test ensures the specific rejection
path and no side-effects.
Contributor
Author
|
Fixing and adjusting based on review once more |
dinahmaccodes
force-pushed
the
issue-23
branch
from
d2d67f6 to
8fbdaad
Compare
Contributor
Author
|
@Bosun-Josh121 |
7 tasks
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.
Closes #23
Description
This PR addresses Issue #23 by implementing an automated, secure expert rate configuration in the payment-vault-contract.
It introduces a proper rate storage mechanism that ensures experts have full control over their own pricing. The book_session function has been updated to verify and calculate deposits entirely based on this trusted on-chain stored rate, eliminating the risk of users providing arbitrary rates.
Finally, a suite of tests has been provided to guarantee deposit calculation accuracy and authorization consistency.
When this command is run, test passes for the changes I made
Summary by CodeRabbit
New Features
Bug Fixes
Tests