Feat/distributor client batch distribute - #592
Open
Fayedamz wants to merge 4 commits into
Open
Conversation
- Add validateContractId() to stream-validation.ts using StrKey.isValidContract
- Apply contract ID validation to token field in paymentStreamSchema (validations.ts)
- Guard token contract address in CreatePaymentStream handleFormSubmit
- Native XLM token ('native') is correctly exempted from contract ID check
Closes Fundable-Protocol#379
Replace all relative path traversals into packages/sdk/src with the workspace alias @fundable/sdk across apps/web: - apps/web/src/lib/api.ts: consolidate PaymentStreamClient, DistributorClient and createBatches into single @fundable/sdk import; remove mid-file stray import - apps/web/src/hooks/use-distribute.ts: createBatches from @fundable/sdk - apps/web/src/hooks/use-distribution-transaction.ts: DistributorClient from @fundable/sdk Relative traversals (../../../../packages/sdk/src/...) bypass pnpm workspace resolution and cause failures in standalone Next.js builds. Closes Fundable-Protocol#368
…-stream Add a global emergency pause switch to the Soroban payment stream contract to halt all payouts and stream creation if a vulnerability is detected. Contract changes (contracts/payment-stream/src/lib.rs): - Add Error variants ContractPaused(17), AlreadyPaused(18), NotPaused(19) - Add EmergencyPausedEvent and EmergencyUnpausedEvent contracttypes - Add private assert_not_paused() guard called at the top of create_stream, deposit, withdraw and withdraw_max - Add emergency_pause(env) – admin-only, sets 'paused' flag in instance storage, emits EmergencyPausedEvent, rejects if already paused - Add emergency_unpause(env) – admin-only, clears 'paused' flag, emits EmergencyUnpausedEvent, rejects if not currently paused - Add is_paused(env) -> bool – read-only, returns current circuit-breaker state - All three new functions are fully doc-commented - Storage uses instance slot with TTL bump, zero extra persistent footprint Test changes (contracts/payment-stream/src/test.rs): - setup_paused_contract() helper to reduce boilerplate across 16 new tests - test_is_paused_default_false - test_emergency_pause_sets_flag - test_emergency_unpause_clears_flag - test_emergency_pause_emits_event - test_emergency_unpause_emits_event - test_emergency_pause_already_paused (#should_panic Fundable-Protocol#18) - test_emergency_unpause_when_not_paused (#should_panic Fundable-Protocol#19) - test_create_stream_blocked_when_paused (#should_panic Fundable-Protocol#17) - test_withdraw_blocked_when_paused (#should_panic Fundable-Protocol#17) - test_withdraw_max_blocked_when_paused (#should_panic Fundable-Protocol#17) - test_deposit_blocked_when_paused (#should_panic Fundable-Protocol#17) - test_operations_resume_after_unpause - test_pause_unpause_cycle (3 cycles) - test_non_admin_cannot_emergency_pause (#should_panic Unauthorized) - test_non_admin_cannot_emergency_unpause (#should_panic Unauthorized) - test_read_operations_work_while_paused Closes Fundable-Protocol#503
Add a single batchDistribute() method on DistributorClient that acts as
a convenient entry point for large-scale distributions. Callers no longer
need to import prepareBatchEqualDistribution / prepareBatchWeightedDistribution
directly from the batchDistribution utils module.
DistributorClient.ts:
- Import BatchDistributionConfig, BatchDistributionResult,
prepareBatchEqualDistribution, prepareBatchWeightedDistribution from
utils/batchDistribution
- Add two typed overload signatures for batchDistribute():
1. Equal distribution: { sender, token, total_amount, recipients, config? }
2. Weighted distribution: { sender, token, recipients, amounts, config? }
- Implementation dispatches to the correct prepare* helper by checking
whether 'amounts' is present in the params object
- Full JSDoc on both overloads with usage examples
__tests__/DistributorClient.test.ts:
- 22 new tests in the batchDistribute describe block
- Equal distribution cases: single batch, multi-batch splitting, default
batch size, total_amount unchanged across batches, empty recipients,
invalid maxRecipientsPerBatch (0, -1), progress callbacks, error
propagation, no amountBatches in result
- Weighted distribution cases: single batch, parallel recipient/amount
splitting, correct slices passed to distributeWeighted, length mismatch
rejection, empty recipients, invalid batch size, error propagation,
progress callbacks, Address object inputs
Closes Fundable-Protocol#194
|
@Fayedamz Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
📝 WalkthroughWalkthroughChangesWeb validation and SDK wiring
Payment-stream emergency circuit breaker
SDK batch distribution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant PaymentStreamContract
participant StreamStorage
Admin->>PaymentStreamContract: emergency_pause()
PaymentStreamContract->>StreamStorage: store paused flag and extend TTL
PaymentStreamContract-->>Admin: publish EmergencyPaused event
Admin->>PaymentStreamContract: create_stream(), deposit(), or withdraw()
PaymentStreamContract->>StreamStorage: read paused flag
PaymentStreamContract-->>Admin: return ContractPaused error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/sdk/src/DistributorClient.tsParsing error: Cannot read file '/tsconfig.json'. packages/sdk/src/__tests__/DistributorClient.test.tsParsing error: Cannot read file '/tsconfig.json'. 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/sdk/src/__tests__/DistributorClient.test.ts (1)
508-519: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't actually pin the default
maxRecipientsPerBatchto 100.With 50 recipients and any default ≥ 50,
batchCountwould still be 1 — this test would pass even if the default were silently changed to, say, 60. Use a recipient count > 100 (e.g., 150) and assertbatchCount === 2with batch sizes[100, 50]to actually verify the documented default of 100.♻️ Proposed strengthening
- it("uses default batch size (100) when config is omitted", async () => { - const recipients = makeRecipients(50); + it("uses default batch size (100) when config is omitted", async () => { + const recipients = makeRecipients(150); const result = await client.batchDistribute({ sender: SENDER, token: TOKEN, - total_amount: 50000n, + total_amount: 150000n, recipients, }); - expect(result.batchCount).toBe(1); - expect(mockContractClient.distribute_equal).toHaveBeenCalledTimes(1); + expect(result.batchCount).toBe(2); + expect(result.recipientBatches[0]).toHaveLength(100); + expect(result.recipientBatches[1]).toHaveLength(50); + expect(mockContractClient.distribute_equal).toHaveBeenCalledTimes(2); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/DistributorClient.test.ts` around lines 508 - 519, Strengthen the “uses default batch size (100) when config is omitted” test by generating 150 recipients, asserting batchCount is 2, and verifying the distribute_equal calls use batch sizes of 100 and 50. Keep the configuration omitted so the test directly pins the default maxRecipientsPerBatch to 100.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/payment-stream/src/lib.rs`:
- Around line 205-209: Correct the pause documentation near the global emergency
pause declaration to enumerate the actual blocked and permitted entry points,
including that cancel_stream, pause_stream, resume_stream, set_delegate, and
revoke_delegate remain callable unless their guards are changed. Keep the
documented behavior aligned with the existing assert_not_paused coverage rather
than claiming only admin operations remain available.
In `@contracts/payment-stream/src/test.rs`:
- Around line 1909-1934: Update test_emergency_pause_emits_event and
test_emergency_unpause_emits_event to import EmergencyPausedEvent and
EmergencyUnpausedEvent from the crate root, then assert the emitted event’s
topic and payload match the corresponding event type and expected values.
Replace the non-specific events.len() > 0 checks while preserving each test’s
existing setup and contract calls.
In `@packages/sdk/src/DistributorClient.ts`:
- Around line 344-354: Update the discriminator in the batch distribution method
around prepareBatchWeightedDistribution to require that params.amounts is an
array, rather than only checking for the amounts key. This must route undefined
or otherwise invalid amounts values to the existing validation path while
preserving weighted distribution for valid arrays.
---
Nitpick comments:
In `@packages/sdk/src/__tests__/DistributorClient.test.ts`:
- Around line 508-519: Strengthen the “uses default batch size (100) when config
is omitted” test by generating 150 recipients, asserting batchCount is 2, and
verifying the distribute_equal calls use batch sizes of 100 and 50. Keep the
configuration omitted so the test directly pins the default
maxRecipientsPerBatch to 100.
🪄 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 Plus
Run ID: ff571eba-9af3-4d96-ac23-7a3895733983
📒 Files selected for processing (10)
apps/web/src/components/modules/payment-stream/CreatePaymentStream.tsxapps/web/src/hooks/use-distribute.tsapps/web/src/hooks/use-distribution-transaction.tsapps/web/src/lib/api.tsapps/web/src/lib/stream-validation.tsapps/web/src/lib/validations.tscontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rspackages/sdk/src/DistributorClient.tspackages/sdk/src/__tests__/DistributorClient.test.ts
Comment on lines
+205
to
+209
| /// Activate the global emergency pause switch. | ||
| /// | ||
| /// When active, all calls to `create_stream`, `deposit`, `withdraw`, and | ||
| /// `withdraw_max` will be rejected with `Error::ContractPaused`. | ||
| /// Admin-only operations (fee management, pause/unpause) remain available. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Doc claims contradict the actual guard scope; cancel_stream still moves funds while paused.
The doc says only admin operations remain available during a pause, but cancel_stream (Line 809) is caller-facing, is not guarded by assert_not_paused, and transfers escrowed tokens out of the contract (Line 845-849). pause_stream, resume_stream, set_delegate, and revoke_delegate also remain callable. Either add the guard to the fund-moving paths you intend to halt, or correct the doc to state exactly which entry points are blocked.
🔧 Option: guard cancel_stream and fix the doc
/// When active, all calls to `create_stream`, `deposit`, `withdraw`, and
- /// `withdraw_max` will be rejected with `Error::ContractPaused`.
- /// Admin-only operations (fee management, pause/unpause) remain available.
+ /// `withdraw_max`, `cancel_stream` will be rejected with
+ /// `Error::ContractPaused`. Stream lifecycle controls (`pause_stream`,
+ /// `resume_stream`), delegation management, and read-only accessors remain
+ /// available, as do admin operations (fee management, pause/unpause).// in cancel_stream
pub fn cancel_stream(env: Env, stream_id: u64) {
Self::assert_not_paused(&env);
...
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payment-stream/src/lib.rs` around lines 205 - 209, Correct the
pause documentation near the global emergency pause declaration to enumerate the
actual blocked and permitted entry points, including that cancel_stream,
pause_stream, resume_stream, set_delegate, and revoke_delegate remain callable
unless their guards are changed. Keep the documented behavior aligned with the
existing assert_not_paused coverage rather than claiming only admin operations
remain available.
Comment on lines
+1909
to
+1934
| /// `emergency_pause` emits the correct event. | ||
| #[test] | ||
| fn test_emergency_pause_emits_event() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let (client, _, _, _, _, _, _) = setup_paused_contract(&env); | ||
| client.emergency_pause(); | ||
|
|
||
| let events = env.events().all(); | ||
| assert!(events.len() > 0); | ||
| } | ||
|
|
||
| /// `emergency_unpause` emits the correct event. | ||
| #[test] | ||
| fn test_emergency_unpause_emits_event() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let (client, _, _, _, _, _, _) = setup_paused_contract(&env); | ||
| client.emergency_pause(); | ||
| client.emergency_unpause(); | ||
|
|
||
| let events = env.events().all(); | ||
| assert!(events.len() > 0); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
These event tests can never fail — assert the actual event.
setup_paused_contract already emits events (initialize/mint), so events.len() > 0 holds even if emergency_pause/emergency_unpause published nothing. The doc comments claim the "correct event" is verified, but neither topic nor payload is checked.
💚 Assert topic and payload instead
fn test_emergency_pause_emits_event() {
let env = Env::default();
env.mock_all_auths();
- let (client, _, _, _, _, _, _) = setup_paused_contract(&env);
+ let (client, contract_id, admin, _, _, _, _) = setup_paused_contract(&env);
client.emergency_pause();
- let events = env.events().all();
- assert!(events.len() > 0);
+ let paused_at = env.ledger().timestamp();
+ let expected = (
+ contract_id,
+ ("EmergencyPaused",).into_val(&env),
+ EmergencyPausedEvent { paused_by: admin, paused_at }.into_val(&env),
+ );
+ assert!(env.events().all().contains(expected));
}EmergencyPausedEvent / EmergencyUnpausedEvent need importing from the crate root for this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payment-stream/src/test.rs` around lines 1909 - 1934, Update
test_emergency_pause_emits_event and test_emergency_unpause_emits_event to
import EmergencyPausedEvent and EmergencyUnpausedEvent from the crate root, then
assert the emitted event’s topic and payload match the corresponding event type
and expected values. Replace the non-specific events.len() > 0 checks while
preserving each test’s existing setup and contract calls.
Comment on lines
+344
to
+354
| ): Promise<BatchDistributionResult> { | ||
| if ("amounts" in params) { | ||
| // Weighted distribution | ||
| return prepareBatchWeightedDistribution(this, { | ||
| sender: params.sender, | ||
| token: params.token, | ||
| recipients: params.recipients, | ||
| amounts: params.amounts, | ||
| config: params.config, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Discriminator doesn't guard against an amounts key present with undefined value.
"amounts" in params returns true even if params.amounts is undefined (e.g., from a spread of a conditionally-built object). That would route into prepareBatchWeightedDistribution, which would throw a confusing TypeError on undefined.length rather than the clear validation errors this API otherwise provides. Consider Array.isArray(params.amounts) for a more robust runtime discriminator.
🛡️ Proposed fix
- if ("amounts" in params) {
+ if (Array.isArray((params as { amounts?: bigint[] }).amounts)) {📝 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
| ): Promise<BatchDistributionResult> { | |
| if ("amounts" in params) { | |
| // Weighted distribution | |
| return prepareBatchWeightedDistribution(this, { | |
| sender: params.sender, | |
| token: params.token, | |
| recipients: params.recipients, | |
| amounts: params.amounts, | |
| config: params.config, | |
| }); | |
| } | |
| ): Promise<BatchDistributionResult> { | |
| if (Array.isArray((params as { amounts?: bigint[] }).amounts)) { | |
| // Weighted distribution | |
| return prepareBatchWeightedDistribution(this, { | |
| sender: params.sender, | |
| token: params.token, | |
| recipients: params.recipients, | |
| amounts: params.amounts, | |
| config: params.config, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/DistributorClient.ts` around lines 344 - 354, Update the
discriminator in the batch distribution method around
prepareBatchWeightedDistribution to require that params.amounts is an array,
rather than only checking for the amounts key. This must route undefined or
otherwise invalid amounts values to the existing validation path while
preserving weighted distribution for valid arrays.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #194. Adds a batchDistribute() method directly on DistributorClient so callers have a single, ergonomic entry point for large distributions without needing to import the lower-level prepareBatch* utilities manually.
What changed
DistributorClient.ts
Imported BatchDistributionConfig, BatchDistributionResult, prepareBatchEqualDistribution, and prepareBatchWeightedDistribution from utils/batchDistribution
Added two typed overload signatures for batchDistribute():
Equal distribution — accepts { sender, token, total_amount, recipients, config? }, dispatches to prepareBatchEqualDistribution
Weighted distribution — accepts { sender, token, recipients, amounts, config? }, dispatches to prepareBatchWeightedDistribution
Dispatch logic is a single "amounts" in params check — no duplication of batching logic, which lives entirely in the existing utility module
Both overloads are fully JSDoc'd with usage examples
DistributorClient.test.ts
Added 22 new tests in a batchDistribute describe block:
Equal distribution: single batch, multi-batch splitting, default batch size (100), total_amount passed unchanged to every batch, empty recipients rejection, invalid maxRecipientsPerBatch (0, -1), onBatchStart/onBatchComplete callbacks, error propagation from distributeEqual, no amountBatches in result.
Weighted distribution: single batch, parallel recipient/amount splitting, correct slices passed to distributeWeighted, length mismatch rejection, empty recipients rejection, non-integer batch size rejection, error propagation from distributeWeighted, progress callbacks, Address object inputs.
Design notes
The existing prepareBatchEqualDistribution and prepareBatchWeightedDistribution functions in batchDistribution.ts were not modified. batchDistribute is purely a dispatch layer — all batching logic, validation, and callback handling remains in the utility module, keeping concerns separated and avoiding duplication.
Testing
No runtime environment was available in this workspace, but all files pass TypeScript diagnostics with zero errors. Tests follow the established vitest + vi.mock patterns used throughout the SDK test suite and can be verified by running pnpm test in packages/sdk/.
closes #194
Summary by CodeRabbit
New Features
Bug Fixes