diff --git a/README.md b/README.md index 4761400b..422a61b2 100644 --- a/README.md +++ b/README.md @@ -184,7 +184,7 @@ fn set_treasury(env, new_treasury: Address) -> Result<(), Error>; (`Contribution { sponsor, amount }`, queryable via `get_contribution`) so `refund` can return each sponsor's own amount to their own address. Capped at `max_sponsors` (an optional `initialize` parameter, defaulting - to `MAX_SPONSORS`, 20) distinct contributions per escrow + to `MAX_SPONSORS`, 20, and limited to 50) distinct contributions per escrow (`TooManySponsors` otherwise). Rejects `AlreadyPaid` / `AlreadyRefunded`. See `docs/escrow-crowdfunding-design.md` for the full design reasoning. - `release`: oracle-only (`require_auth` on the stored oracle address). @@ -278,8 +278,9 @@ fn set_treasury(env, new_treasury: Address) -> Result<(), Error>; }`, queryable via `get_contribution`, with the original funder always at index 0) so a cancellation refund can return each sponsor's proportional share to their own address. Capped at `max_sponsors` (an - optional `initialize` parameter, defaulting to `MAX_SPONSORS`, 20) - distinct contributions per milestone (`TooManySponsors` otherwise). + optional `initialize` parameter, defaulting to `MAX_SPONSORS`, 20) and + limited to 50 distinct contributions per milestone (`TooManySponsors` + otherwise). Rejects `MilestoneClosed`. See `docs/milestones-crowdfunding-design.md` for the full design reasoning. - `allocate`: admin-only. Reserves a slice of `remaining_budget` for a @@ -708,6 +709,9 @@ Stellar-side problem), so `scripts/deploy.mjs` and `scripts/invoke.mjs` (thin wrappers around `@stellar/stellar-sdk`) were used instead to perform the same upload → create-contract → initialize flow the CLI would otherwise do. +For payout methods such as `release` and `release_issue`, `invoke.mjs` +accepts recipient splits as `recipients:
=,
=` +(for example, `recipients:G...=7000,G...=3000`). | Contract | Contract ID | Stellar Expert | |---|---|---| diff --git a/contracts/common/src/lib.rs b/contracts/common/src/lib.rs index c6a1a7a6..968a78cc 100644 --- a/contracts/common/src/lib.rs +++ b/contracts/common/src/lib.rs @@ -15,6 +15,10 @@ pub use split::{compute_split, sort_remainders_desc, Payouts, SplitError}; #[cfg(test)] mod test_fuzz; +/// Maximum accepted per-contract sponsor cap. This keeps contributor loops +/// within a predictable resource budget even when deployments customize it. +pub const MAX_SPONSORS_LIMIT: u32 = 50; + /// Trait to identify the Admin key for a contract's DataKey enum. pub trait AdminKey { /// Returns the storage key for the admin address. diff --git a/contracts/common/src/split.rs b/contracts/common/src/split.rs index 100f8439..9220f728 100644 --- a/contracts/common/src/split.rs +++ b/contracts/common/src/split.rs @@ -110,7 +110,8 @@ pub fn compute_split( .ok_or(SplitError::Overflow)?; if dust > 0 { sort_remainders_desc(&mut order); - for k in 0..dust as u32 { + let dust = u32::try_from(dust).map_err(|_| SplitError::Overflow)?; + for k in 0..dust { let (index, _, _) = order.get(k).unwrap(); let (recipient, share) = shares.get(index).unwrap(); shares.set( diff --git a/contracts/escrow/src/error.rs b/contracts/escrow/src/error.rs index 4c130c24..16b73c69 100644 --- a/contracts/escrow/src/error.rs +++ b/contracts/escrow/src/error.rs @@ -35,4 +35,6 @@ pub enum Error { ArithmeticOverflow = 21, /// The stored contract version does not match the requested migration source. InvalidVersion = 22, + /// `initialize` supplied a sponsor cap above `MAX_SPONSORS_LIMIT`. + InvalidMaxSponsors = 23, } diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 4096f556..7b66ff4e 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -15,7 +15,7 @@ mod types; mod test; use error::Error; -use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS}; +use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS, MAX_SPONSORS_LIMIT}; use soroban_sdk::{contract, contractevent, contractimpl, token, Address, BytesN, Env, Vec}; use types::{Contribution, DataKey, Escrow, EscrowStatus}; @@ -206,7 +206,8 @@ mod contract { /// /// `max_sponsors` caps how many distinct contributions a single escrow /// may accumulate (see `contribute`); pass `None` to use the default - /// `MAX_SPONSORS` (20). + /// `MAX_SPONSORS` (20). Custom values may not exceed + /// `MAX_SPONSORS_LIMIT` (50), keeping contributor loops bounded. pub fn initialize( env: Env, admin: Address, @@ -228,12 +229,15 @@ mod contract { if treasury == env.current_contract_address() { return Err(Error::InvalidTreasury); } + let max_sponsors = max_sponsors.unwrap_or(MAX_SPONSORS); + if max_sponsors > MAX_SPONSORS_LIMIT { + return Err(Error::InvalidMaxSponsors); + } env.storage().instance().set(&DataKey::Admin, &admin); env.storage().instance().set(&DataKey::Oracle, &oracle); env.storage().instance().set(&DataKey::Treasury, &treasury); env.storage().instance().set(&DataKey::FeeBps, &fee_bps); - let max_sponsors = max_sponsors.unwrap_or(MAX_SPONSORS); env.storage() .instance() .set(&DataKey::MaxSponsors, &max_sponsors); diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index 47a07d1e..22b9c1d6 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -1,6 +1,7 @@ #![cfg(test)] use super::*; +use mergefi_common::MAX_SPONSORS_LIMIT; use soroban_sdk::{ testutils::{storage::Persistent as _, Address as _, Ledger}, token, vec, Address, Env, Event, @@ -1254,6 +1255,37 @@ fn test_initialize_accepts_a_custom_max_sponsors() { assert_eq!(err, Err(Ok(Error::TooManySponsors))); } +#[test] +fn test_initialize_caps_max_sponsors_at_shared_limit() { + let env = Env::default(); + env.mock_all_auths(); + let admin = Address::generate(&env); + let oracle = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(EscrowContract, ()); + let client = EscrowContractClient::new(&env, &contract_id); + + let err = client.try_initialize( + &admin, + &oracle, + &treasury, + &500u32, + &Some(MAX_SPONSORS_LIMIT + 1), + &None, + ); + assert_eq!(err, Err(Ok(Error::InvalidMaxSponsors))); + + client.initialize( + &admin, + &oracle, + &treasury, + &500u32, + &Some(MAX_SPONSORS_LIMIT), + &None, + ); + assert_eq!(client.get_max_sponsors(), MAX_SPONSORS_LIMIT); +} + #[test] fn test_extend_deadline_rejects_later_contributor_only_original_sponsor_can_extend() { let env = Env::default(); diff --git a/contracts/milestones/src/error.rs b/contracts/milestones/src/error.rs index 753ea058..d1f34d8d 100644 --- a/contracts/milestones/src/error.rs +++ b/contracts/milestones/src/error.rs @@ -45,4 +45,6 @@ pub enum Error { DeadlineInPast = 22, /// The stored contract version does not match the requested migration source. InvalidVersion = 23, + /// `initialize` supplied a sponsor cap above `MAX_SPONSORS_LIMIT`. + InvalidMaxSponsors = 24, } diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 35b8eaab..49682efb 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -22,7 +22,7 @@ use error::Error; use soroban_sdk::{contract, contractimpl, token, Address, BytesN, Env, Map, Vec}; use types::{Contribution, DataKey, IssueStatus, Milestone}; -use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS}; +use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS, MAX_SPONSORS_LIMIT}; /// Minimum grace period (in seconds) after the deadline before anyone can /// permissionlessly trigger a cancel_milestone. Mirrors escrow's @@ -53,7 +53,8 @@ mod contract { /// /// `max_sponsors` caps how many distinct contributions a single /// milestone may accumulate (see `contribute`); pass `None` to use the - /// default `MAX_SPONSORS` (20). + /// default `MAX_SPONSORS` (20). Custom values may not exceed + /// `MAX_SPONSORS_LIMIT` (50), keeping contributor loops bounded. pub fn initialize( env: Env, admin: Address, @@ -75,13 +76,17 @@ mod contract { if treasury == env.current_contract_address() { return Err(Error::InvalidTreasury); } + let max_sponsors = max_sponsors.unwrap_or(MAX_SPONSORS); + if max_sponsors > MAX_SPONSORS_LIMIT { + return Err(Error::InvalidMaxSponsors); + } env.storage().instance().set(&DataKey::Admin, &admin); env.storage().instance().set(&DataKey::Oracle, &oracle); env.storage().instance().set(&DataKey::Treasury, &treasury); env.storage().instance().set(&DataKey::FeeBps, &fee_bps); env.storage() .instance() - .set(&DataKey::MaxSponsors, &max_sponsors.unwrap_or(MAX_SPONSORS)); + .set(&DataKey::MaxSponsors, &max_sponsors); env.storage() .instance() .set(&DataKey::Version, &CONTRACT_VERSION); @@ -957,7 +962,8 @@ fn refund_remaining_budget( .ok_or(Error::ArithmeticOverflow)?; if dust > 0 { mergefi_common::sort_remainders_desc(&mut order); - for k in 0..dust as u32 { + let dust = u32::try_from(dust).map_err(|_| Error::ArithmeticOverflow)?; + for k in 0..dust { let (index, _, _) = order.get(k).unwrap(); let (recipient, share) = shares.get(index).unwrap(); shares.set( diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 21cd988c..c13e68a9 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -1,7 +1,7 @@ #![cfg(test)] use super::*; -use mergefi_common::MAX_SPONSORS; +use mergefi_common::{MAX_SPONSORS, MAX_SPONSORS_LIMIT}; use soroban_sdk::{ testutils::{Address as _, Ledger as _}, token, vec, Address, Env, @@ -754,6 +754,37 @@ fn test_initialize_accepts_a_custom_max_sponsors() { assert_eq!(err, Err(Ok(Error::TooManySponsors))); } +#[test] +fn test_initialize_caps_max_sponsors_at_shared_limit() { + let env = Env::default(); + env.mock_all_auths(); + let admin = Address::generate(&env); + let oracle = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(MilestonesContract, ()); + let client = MilestonesContractClient::new(&env, &contract_id); + + let err = client.try_initialize( + &admin, + &oracle, + &treasury, + &500u32, + &Some(MAX_SPONSORS_LIMIT + 1), + &None, + ); + assert_eq!(err, Err(Ok(Error::InvalidMaxSponsors))); + + client.initialize( + &admin, + &oracle, + &treasury, + &500u32, + &Some(MAX_SPONSORS_LIMIT), + &None, + ); + assert_eq!(client.get_max_sponsors(), MAX_SPONSORS_LIMIT); +} + #[test] fn test_get_contribution_enumerates_each_contributor() { let env = Env::default(); diff --git a/docs/escrow-crowdfunding-design.md b/docs/escrow-crowdfunding-design.md index 8c9fcdcd..61f50a46 100644 --- a/docs/escrow-crowdfunding-design.md +++ b/docs/escrow-crowdfunding-design.md @@ -56,16 +56,16 @@ Two reasons: don't need it at all. Per-index entries keep `Escrow` itself small and let `refund`/`extend_deadline` read only what they need. 2. **No unbounded growth.** `max_sponsors` (an optional `initialize` - parameter, defaulting to the `MAX_SPONSORS` constant, 20, when omitted) + parameter, defaulting to the `MAX_SPONSORS` constant, 20, when omitted + and capped at 50) caps `contributor_count`, so `refund`'s and `extend_deadline`'s per-contributor loops are bounded by a small, per-deployment-tunable constant regardless of how popular a bounty gets — directly addressing the same resource concern #8/#9 raise for `recipients`/`allocations` - elsewhere in this codebase. Twenty is generous enough for any realistic - crowdfunding scenario for a single GitHub issue while keeping the - worst-case loop (and its Stellar resource cost) small and predictable; - a deployment expecting unusually high participation can raise the cap - at `initialize` time instead of needing a contract redeploy (#96). + elsewhere in this codebase. The default of 20 is generous enough for + realistic crowdfunding on a single GitHub issue; deployments can raise + it to at most 50 at `initialize` time, keeping the worst-case loop (and + its Stellar resource cost) bounded without requiring a redeploy (#96, #448). Because each contribution is recorded as an *exact* amount rather than a percentage, `refund` needs no proportional-split math at all: it iterates @@ -107,4 +107,3 @@ arbitrarily far out in one call. It can be extended again later, still only up to 365 days ahead of that later call. The admin's independent early-refund path (`refund` before `deadline + GRACE_PERIOD`, admin-only) remains available as an escape hatch. - diff --git a/docs/milestones-crowdfunding-design.md b/docs/milestones-crowdfunding-design.md index 1d9be8b3..3b131d92 100644 --- a/docs/milestones-crowdfunding-design.md +++ b/docs/milestones-crowdfunding-design.md @@ -102,14 +102,14 @@ inline on `Milestone`. Two reasons, unchanged from escrow: history, even for operations (`allocate`, `release_issue`) that never look at it. 2. **No unbounded growth.** `max_sponsors` (an optional `initialize` - parameter, defaulting to the `MAX_SPONSORS` constant, 20, when omitted) + parameter, defaulting to the `MAX_SPONSORS` constant, 20, when omitted + and capped at 50) caps `contributor_count`, so `cancel_milestone`'s (and any future timeout wind-down's) per-contributor loop — and, critically, the refund's dust distribution — is bounded by a small, per-deployment- - tunable constant regardless of how popular a release gets. A $50,000 - release milestone expecting far more participation than a single-issue - bounty can raise the cap at `initialize` time instead of needing a - contract redeploy (#96). + tunable constant regardless of how popular a release gets. A release + milestone with unusually high participation can raise the cap to at most + 50 at `initialize` time instead of needing a contract redeploy (#96, #449). ## Authorization diff --git a/scripts/error-codes.mjs b/scripts/error-codes.mjs index 93d47ce6..920ab043 100644 --- a/scripts/error-codes.mjs +++ b/scripts/error-codes.mjs @@ -24,7 +24,8 @@ export const errorMappings = { 16: "InvalidTarget", 17: "InvalidTreasury", 19: "ContractPaused", - 20: "SelfPayout" + 20: "SelfPayout", + 23: "InvalidMaxSponsors" }, // Milestones contract errors @@ -48,7 +49,8 @@ export const errorMappings = { 17: "IssueNotAllocatedForDeallocate", 18: "ContractPaused", 19: "ContributionNotFound", - 20: "SelfPayout" + 20: "SelfPayout", + 24: "InvalidMaxSponsors" }, // Maintenance-pool contract errors diff --git a/scripts/invoke.mjs b/scripts/invoke.mjs index daba36df..042b2836 100644 --- a/scripts/invoke.mjs +++ b/scripts/invoke.mjs @@ -7,6 +7,7 @@ import { Address, nativeToScVal, rpc, + xdr, } from "@stellar/stellar-sdk"; import { submitAndWait } from "./lib/submit.mjs"; import { formatError } from "./error-codes.mjs"; @@ -35,6 +36,27 @@ export function parseArg(raw) { const [type, value] = raw.split(":"); if (type === "address") return nativeToScVal(new Address(value), { type: "address" }); + if (type === "recipients") { + if (!value) { + throw new Error("Invalid recipients argument: expected address=u32,..."); + } + + const recipients = value.split(",").map((entry) => { + const [address, bps, ...extra] = entry.split("="); + if (!address || !bps || extra.length > 0 || !/^\d+$/.test(bps)) { + throw new Error(`Invalid recipients entry: ${entry}`); + } + const share = Number(bps); + if (!Number.isSafeInteger(share) || share > 0xffffffff) { + throw new Error(`Recipient basis points must fit u32: ${bps}`); + } + return xdr.ScVal.scvVec([ + nativeToScVal(new Address(address), { type: "address" }), + nativeToScVal(share, { type: "u32" }), + ]); + }); + return xdr.ScVal.scvVec(recipients); + } if (type === "u32") return nativeToScVal(parseInt(value, 10), { type: "u32" }); if (type === "u64") return nativeToScVal(BigInt(value), { type: "u64" }); if (type === "i128" || type === "some-i128") { diff --git a/scripts/invoke.test.mjs b/scripts/invoke.test.mjs index 2e34f2c8..3ccb6c14 100644 --- a/scripts/invoke.test.mjs +++ b/scripts/invoke.test.mjs @@ -2,6 +2,7 @@ import { test, describe } from "node:test"; import assert from "node:assert/strict"; import { execFile } from "node:child_process"; import { promisify } from "node:util"; +import { Address, nativeToScVal } from "@stellar/stellar-sdk"; import { parseArg } from "./invoke.mjs"; const execFileAsync = promisify(execFile); @@ -39,6 +40,29 @@ describe("scripts/invoke.mjs parseArg unit tests", () => { assert.ok(scVal); }); + test("parses recipients as a vector of address/u32 tuple vectors", () => { + const address1 = "GBZXN7PIRZGNMHGA72BLBHNTFAJUR3NGMHHCC5YDHJL3PPCHQSTFU7H6"; + const address2 = "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF"; + const recipients = parseArg(`recipients:${address1}=7000,${address2}=3000`).vec(); + + assert.equal(recipients.length, 2); + assert.equal(recipients[0].vec().length, 2); + assert.ok( + recipients[0] + .vec()[0] + .toXDR() + .equals(nativeToScVal(new Address(address1), { type: "address" }).toXDR()) + ); + assert.equal(recipients[0].vec()[1].u32(), 7000); + assert.equal(recipients[1].vec()[1].u32(), 3000); + }); + + test("rejects malformed recipient vectors", () => { + assert.throws(() => parseArg("recipients:"), /expected address=u32/); + assert.throws(() => parseArg("recipients:G...=NaN"), /Invalid recipients entry/); + assert.throws(() => parseArg("recipients:G...=4294967296"), /fit u32/); + }); + test("parses u32 correctly", () => { const scVal = parseArg("u32:250"); assert.ok(scVal);