feat: implemented the multi-sig control for protocol fee parameters#664
feat: implemented the multi-sig control for protocol fee parameters#664CollinsC1O wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis pull request implements fee governance multisig by replacing single-admin fee configuration with a two-function approach: Changes
Sequence Diagram(s)sequenceDiagram
participant Admin1 as Admin 1
participant Admin2 as Admin 2
participant Contract as Fee Config Contract
participant Storage as Contract Storage
rect rgba(100, 149, 237, 0.5)
Note over Admin1,Storage: Setup Phase
Admin1->>Contract: configure_fee_multisig(admins=[Admin1, Admin2], threshold=2)
Contract->>Contract: Validate no duplicates & threshold ≤ admin count
Contract->>Storage: Store admin set & approval threshold
end
rect rgba(34, 139, 34, 0.5)
Note over Admin1,Storage: Approval Phase
Admin1->>Contract: configure_fees(fee_bips, treasury_addr) with Admin1 signature
Contract->>Contract: Validate Admin1 is in admin set
Contract->>Storage: Record Admin1 approval
Admin2->>Contract: configure_fees(fee_bips, treasury_addr) with Admin2 signature
Contract->>Contract: Validate Admin2 is in admin set
Contract->>Contract: Check if threshold (2) approvals reached
Contract->>Storage: Update FEE_BIPS & TREASURY_ADDR
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
@CollinsC1O 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! 🚀 |
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 (2)
contracts/game_contract/src/lib.rs (2)
1811-1834:⚠️ Potential issue | 🔴 CriticalCritical: test calls
resolve_disputetwice and asserts two contradictory balances.
client.resolve_dispute(...)is invoked at lines 1812 and again at 1821 for the samedispute_id. After the first call the dispute status becomesResolved, so the second call must returnErr(ContractError::GameAlreadyCompleted)per the guard at line 1130. The test then assertsbalance(&player1) == 1_050(line 1829) andbalance(&player1) == 1_100(line 1834) — these cannot both hold. This is a merge artifact that mirrors the duplicated logic inresolve_disputeitself and should be reduced to a single call with a single assertion (fee = 0 for this test → winner receives full2 * wager = 200, so1000 - 100 + 200 = 1_100).🛠️ Proposed fix
let reason = Bytes::from_slice(&env, b"Illegal move"); let dispute_id = client.file_dispute(&game_id, &player1, &player2, &reason); - let resolution = Bytes::from_slice(&env, b"Awarding win to player1"); + + // Arbitrator resolves in favor of player1 + let resolution = Bytes::from_slice(&env, b"Player1 wins"); client.resolve_dispute( &dispute_id, &arbitrator, &Some(player1.clone()), &resolution, ); - // Arbitrator resolves in favor of player1 - let resolution = Bytes::from_slice(&env, b"Player1 wins"); - client.resolve_dispute( - &dispute_id, - &arbitrator, - &Some(player1.clone()), - &resolution, - ); - - // Verify player1 received the payout - assert_eq!(token_client.balance(&player1), 1_050); - - // Verify dispute is resolved let dispute = client.get_dispute(&dispute_id); assert_eq!(dispute.status, DisputeStatus::Resolved); assert_eq!(token_client.balance(&player1), 1_100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 1811 - 1834, The test erroneously calls client.resolve_dispute(...) twice for the same dispute_id and asserts contradictory balances; remove the duplicate resolution so resolve_dispute is invoked only once (keep the call that resolves in favor of player1), ensure you check dispute status via client.get_dispute(...) equals DisputeStatus::Resolved, and update the token_client.balance(&player1) assertion to a single expected value of 1_100 (reflecting full payout with fee=0).
1139-1210:⚠️ Potential issue | 🔴 CriticalCritical:
resolve_disputewill not compile due to use-after-move and has a broken control-flow structure.This function contains a failed merge that causes multiple compile-time failures:
Use-after-move errors (lines 1199–1202):
- Line 1147 moves
disputeintodisputes.set(dispute_id, dispute)- Lines 1199–1201 then attempt to reassign and move
disputeagain, which will fail to compileBroken else-block structure (lines 1167–1176):
- Line 1167 opens
} else {, and lines 1168–1175 are properly indented inside it- Line 1176 (
if game.state != GameState::InProgress) is indented at the wrong level (8 spaces instead of 12), suggesting the else block has closed without a visible}- Yet the code references
gamecreated at line 1175 within that closed block, creating a scope violation- Brace count confirms imbalance: 10 open, 9 close
Duplicate game settlement and payout:
- Lines 1151–1166 already handle the
Some(winner)case with aprocess_payoutcall (line 1163)- Lines 1176–1194 repeat the settlement logic with another payout call (line 1187), causing double escrow drain if both paths execute
The entire settlement block from lines 1143–1202 must be rewritten as a single, linear flow. Remove the redundant early settlement attempt (lines 1143–1166) and consolidate into one game-state check and payout dispatch:
Suggested structure
- // Update dispute status - dispute.status = DisputeStatus::Resolved; - dispute.resolution = Some(resolution.clone()); - let game_id = dispute.game_id; - disputes.set(dispute_id, dispute); - env.storage().instance().set(&DISPUTES, &disputes); - - // Update game state and process payout based on arbitrator's decision - if let Some(ref winner_addr) = winner { - // Winner takes all - let mut games: Map<u64, Game> = env - .storage() - .instance() - .get(&GAMES) - .ok_or(ContractError::GameNotFound)?; - - let mut game = games.get(game_id).ok_or(ContractError::GameNotFound)?; - - game.state = GameState::Completed; - game.winner = Some(winner_addr.clone()); - Self::process_payout(&env, &game, winner_addr)?; - - games.set(game_id, game); - env.storage().instance().set(&GAMES, &games); - } else { - // Draw - refund both players - let mut games: Map<u64, Game> = env - .storage() - .instance() - .get(&GAMES) - .ok_or(ContractError::GameNotFound)?; - - let game = games.get(game_id).ok_or(ContractError::GameNotFound)?; - if game.state != GameState::InProgress { + if game.state != GameState::InProgress { return Err(ContractError::GameAlreadyCompleted); } match winner { Some(ref winner_addr) => { if *winner_addr != game.player1 && Some(winner_addr.clone()) != game.player2 { return Err(ContractError::NotPlayer); } game.state = GameState::Completed; game.winner = Some(winner_addr.clone()); Self::process_payout(&env, &game, winner_addr)?; } None => { game.state = GameState::Drawn; game.winner = None; Self::process_draw_payout(&env, &game)?; } } games.set(dispute.game_id, game); env.storage().instance().set(&GAMES, &games); dispute.status = DisputeStatus::Resolved; dispute.resolution = Some(resolution); disputes.set(dispute_id, dispute); env.storage().instance().set(&DISPUTES, &disputes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 1139 - 1210, The resolve_dispute branch has a failed merge: remove the duplicated early settlement and rewrite the block as one linear flow to avoid use-after-move and brace/scope errors; specifically, in resolve_dispute, stop calling disputes.set(dispute_id, dispute) and env.storage().instance().set(&DISPUTES, &disputes) before finishing game handling, instead: update dispute.status and dispute.resolution in-place (or clone resolution if needed), then load mutable games (GAMES), get a mutable game by game_id, check game.state == GameState::InProgress, match on winner to set game.state/Game.winner and call either Self::process_payout(&env, &game, winner_addr) or Self::process_draw_payout(&env, &game), then games.set(game_id, game) and persist GAMES and finally persist DISPUTES once (disputes.set and env.storage().instance().set(&DISPUTES,...)) and publish the event; ensure you reference resolve_dispute, dispute/dispute_id, disputes/DISPUTES, game/game_id, GAMES, GameState, process_payout and process_draw_payout while fixing braces and removing the duplicated payout path so no use-after-move or double payout occurs.
🧹 Nitpick comments (4)
contracts/game_contract/src/lib.rs (3)
100-101: Storage keys LGTM; consider persistent vs. instance storage for the fee-admin set.
FEE_ADMINSandFEE_THRESHOLDare written viaenv.storage().instance()(lines 706-707, 730). Instance storage is rent-paid with the contract and is appropriate here, but note that a growing admin set contributes to the instance footprint and its rent cost. If the admin set is expected to stay small (≤ ~20) this is fine; otherwisepersistentstorage with explicit TTL management may be more appropriate. No change required — just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 100 - 101, Storage keys FEE_ADMINS and FEE_THRESHOLD are currently stored via env.storage().instance(), which is fine for a small admin set but will increase instance rent as the set grows; if you expect the admin set to exceed ~20 entries or need TTL management, migrate storage of FEE_ADMINS to persistent storage (use persistent APIs instead of env.storage().instance() where FEE_ADMINS is read/written) and keep FEE_THRESHOLD in instance storage if it remains a small scalar, otherwise move both to persistent with explicit TTL handling to control rent costs.
677-708:callerparameter is never authenticated — onlyCONTRACT_ADMINis.
current_admin.require_auth()is invoked, butcaller.require_auth()is not; thecaller != current_adminequality check is then essentially a no-op once they are required to be equal. Thecallerparameter is effectively dead: callers can pass any address without consequence, and the security guarantee comes solely from theCONTRACT_ADMINauth. Either dropcallerand document this as aCONTRACT_ADMIN-only operation, orrequire_authoncallerand use it for the authority check.Additionally,
unique_setis built only as a scratch dedup tracker and then discarded;new_adminsis what gets stored. That is fine, but consider storingunique_setinstead to avoid any possibility of persisting a duplicate if the validation invariant is ever relaxed.🛠️ Suggested simplification
- pub fn configure_fee_multisig( - env: Env, - caller: Address, - new_admins: Vec<Address>, - threshold: u32, - ) { - let current_admin: Address = env - .storage() - .instance() - .get(&CONTRACT_ADMIN) - .expect("Not initialized"); - current_admin.require_auth(); - - if caller != current_admin { - panic!("Unauthorized admin address"); - } + pub fn configure_fee_multisig( + env: Env, + new_admins: Vec<Address>, + threshold: u32, + ) { + let current_admin: Address = env + .storage() + .instance() + .get(&CONTRACT_ADMIN) + .expect("Not initialized"); + current_admin.require_auth();Note: this is a public-API change; align the client/tests accordingly if adopted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 677 - 708, configure_fee_multisig currently calls current_admin.require_auth() but never authenticates the caller parameter, making the caller check ineffective; update configure_fee_multisig to require_auth() on the caller Address and use that authenticated caller for the admin equality check (or alternatively remove the caller parameter and document the function as CONTRACT_ADMIN-only), and, after deduplicating admins in the unique_set inside configure_fee_multisig, persist unique_set (not the original new_admins) to FEE_ADMINS to ensure no duplicates can be stored; reference function configure_fee_multisig, symbols caller, current_admin, unique_set, FEE_ADMINS, and FEE_THRESHOLD when making the change.
710-765:configure_feessilently falls back to single-admin mode and lacks afee_bipslower-bound sanity check.Two concerns on the new multisig entry point:
- When
FEE_ADMINShas never been set,stored_adminsis synthesized fromCONTRACT_ADMINandthresholddefaults to1. That is a reasonable bootstrap, but it means the multisig protection is effectively opt-in: if an operator forgets to callconfigure_fee_multisig, a singleCONTRACT_ADMINsignature can still modify fees — defeating the stated goal of issue#515. Consider either (a) requiringconfigure_fee_multisigto have been called beforeconfigure_feesis usable, or (b) emitting an event / exposing a read method so off-chain monitoring can detect the un-initialized state. At minimum, document this behavior in the function's docstring.fee_bipsis only upper-bounded (> 1000).initialize_puzzle_rewardsenforces the same bound, so this is consistent — fine to leave, but consider extracting a sharedvalidate_fee_bipshelper to keep the invariant DRY.Also minor:
unique_approvalsalways equalsapproved_set.len()after the loop, so it is redundant; you can drop it and checkapproved_set.len() < thresholddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 710 - 765, configure_fees currently synthesizes stored_admins from CONTRACT_ADMIN when FEE_ADMINS is unset (making multisig effectively opt-in), lacks a lower-bound sanity check for fee_bips, and keeps a redundant unique_approvals counter; fix by (1) explicitly requiring FEE_ADMINS to be initialized (or at minimum emitting an on-chain event / setting a boolean flag) before allowing multisig changes instead of falling back to CONTRACT_ADMIN—detect FEE_ADMINS presence instead of synthesizing from CONTRACT_ADMIN inside configure_fees; (2) add a shared helper validate_fee_bips (used by configure_fees and initialize_puzzle_rewards) that enforces 0 <= fee_bips <= 1000 and call it before persisting FEE_BIPS; and (3) remove the redundant unique_approvals variable and use approved_set.len() for threshold checks (references: function configure_fees, stored_admins/FEE_ADMINS, CONTRACT_ADMIN, FEE_THRESHOLD, fee_bips, initialize_puzzle_rewards, validate_fee_bips, approved_set).contracts/game_contract/src/test.rs (1)
308-320: Test correctly exercises the pre-multisig fallback path.Passing
Vec::from([admin])relies onconfigure_feesfalling back toCONTRACT_ADMINwhenFEE_ADMINSis not configured; the stranger-path test still correctly assertsErr. Consider adding a companion test that exercises the post-configure_fee_multisigM-of-N flow (e.g., threshold=2, 3 admins, verify 1 approval panics, 2 approvals succeed, duplicate approval panics) to cover the new multi-sig logic that currently has no dedicated test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/test.rs` around lines 308 - 320, Add a new test that exercises the post-configure_fee_multisig M-of-N flow: call client.configure_fee_multisig to set threshold=2 with 3 admin addresses (use Vec::from([...]) or Vec::new and push_back), then attempt to apply configure_fees with only one approval and assert it fails/panics, then submit a second distinct approval (via the client.approve_configure_fees or related approval method) and assert the configure_fees action now succeeds, and finally assert a duplicate approval is rejected (panics or returns Err). Ensure the test still includes the existing stranger/CONTRACT_ADMIN fallback check and references FEE_ADMINS where appropriate so both pre-multisig and post-multisig paths are covered.
🤖 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/game_contract/src/lib.rs`:
- Around line 1751-1752: Tests call configure_dispute_system twice in a row
(configure_dispute_system(&admin, &arbitrator, &50i128); then
configure_dispute_system(&admin, &arbitrator, &25i128); or 50 then 0), so the
second call overrides the first; remove the unintended duplicate call so only
the intended dispute_fee is set (decide whether the test should use 50, 25, or
0), update the test to call configure_dispute_system only once (referencing the
configure_dispute_system(...) invocation), and then verify the test assertions
that depend on the fee (expected balances like 875 and 900) match the retained
dispute_fee; apply the same change for the other duplicate occurrences of
configure_dispute_system in the file.
---
Outside diff comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 1811-1834: The test erroneously calls client.resolve_dispute(...)
twice for the same dispute_id and asserts contradictory balances; remove the
duplicate resolution so resolve_dispute is invoked only once (keep the call that
resolves in favor of player1), ensure you check dispute status via
client.get_dispute(...) equals DisputeStatus::Resolved, and update the
token_client.balance(&player1) assertion to a single expected value of 1_100
(reflecting full payout with fee=0).
- Around line 1139-1210: The resolve_dispute branch has a failed merge: remove
the duplicated early settlement and rewrite the block as one linear flow to
avoid use-after-move and brace/scope errors; specifically, in resolve_dispute,
stop calling disputes.set(dispute_id, dispute) and
env.storage().instance().set(&DISPUTES, &disputes) before finishing game
handling, instead: update dispute.status and dispute.resolution in-place (or
clone resolution if needed), then load mutable games (GAMES), get a mutable game
by game_id, check game.state == GameState::InProgress, match on winner to set
game.state/Game.winner and call either Self::process_payout(&env, &game,
winner_addr) or Self::process_draw_payout(&env, &game), then games.set(game_id,
game) and persist GAMES and finally persist DISPUTES once (disputes.set and
env.storage().instance().set(&DISPUTES,...)) and publish the event; ensure you
reference resolve_dispute, dispute/dispute_id, disputes/DISPUTES, game/game_id,
GAMES, GameState, process_payout and process_draw_payout while fixing braces and
removing the duplicated payout path so no use-after-move or double payout
occurs.
---
Nitpick comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 100-101: Storage keys FEE_ADMINS and FEE_THRESHOLD are currently
stored via env.storage().instance(), which is fine for a small admin set but
will increase instance rent as the set grows; if you expect the admin set to
exceed ~20 entries or need TTL management, migrate storage of FEE_ADMINS to
persistent storage (use persistent APIs instead of env.storage().instance()
where FEE_ADMINS is read/written) and keep FEE_THRESHOLD in instance storage if
it remains a small scalar, otherwise move both to persistent with explicit TTL
handling to control rent costs.
- Around line 677-708: configure_fee_multisig currently calls
current_admin.require_auth() but never authenticates the caller parameter,
making the caller check ineffective; update configure_fee_multisig to
require_auth() on the caller Address and use that authenticated caller for the
admin equality check (or alternatively remove the caller parameter and document
the function as CONTRACT_ADMIN-only), and, after deduplicating admins in the
unique_set inside configure_fee_multisig, persist unique_set (not the original
new_admins) to FEE_ADMINS to ensure no duplicates can be stored; reference
function configure_fee_multisig, symbols caller, current_admin, unique_set,
FEE_ADMINS, and FEE_THRESHOLD when making the change.
- Around line 710-765: configure_fees currently synthesizes stored_admins from
CONTRACT_ADMIN when FEE_ADMINS is unset (making multisig effectively opt-in),
lacks a lower-bound sanity check for fee_bips, and keeps a redundant
unique_approvals counter; fix by (1) explicitly requiring FEE_ADMINS to be
initialized (or at minimum emitting an on-chain event / setting a boolean flag)
before allowing multisig changes instead of falling back to
CONTRACT_ADMIN—detect FEE_ADMINS presence instead of synthesizing from
CONTRACT_ADMIN inside configure_fees; (2) add a shared helper validate_fee_bips
(used by configure_fees and initialize_puzzle_rewards) that enforces 0 <=
fee_bips <= 1000 and call it before persisting FEE_BIPS; and (3) remove the
redundant unique_approvals variable and use approved_set.len() for threshold
checks (references: function configure_fees, stored_admins/FEE_ADMINS,
CONTRACT_ADMIN, FEE_THRESHOLD, fee_bips, initialize_puzzle_rewards,
validate_fee_bips, approved_set).
In `@contracts/game_contract/src/test.rs`:
- Around line 308-320: Add a new test that exercises the
post-configure_fee_multisig M-of-N flow: call client.configure_fee_multisig to
set threshold=2 with 3 admin addresses (use Vec::from([...]) or Vec::new and
push_back), then attempt to apply configure_fees with only one approval and
assert it fails/panics, then submit a second distinct approval (via the
client.approve_configure_fees or related approval method) and assert the
configure_fees action now succeeds, and finally assert a duplicate approval is
rejected (panics or returns Err). Ensure the test still includes the existing
stranger/CONTRACT_ADMIN fallback check and references FEE_ADMINS where
appropriate so both pre-multisig and post-multisig paths are covered.
🪄 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: ffc960e1-39a2-401c-ad4c-d31aec6fc5a3
📒 Files selected for processing (2)
contracts/game_contract/src/lib.rscontracts/game_contract/src/test.rs
| client.configure_dispute_system(&admin, &arbitrator, &50i128); | ||
| client.configure_dispute_system(&admin, &arbitrator, &25i128); |
There was a problem hiding this comment.
Duplicate configure_dispute_system calls in three dispute tests.
Each of these tests calls configure_dispute_system twice back-to-back with different dispute_fee values (50 then 25, or 50 then 0). The second call silently overrides the first, so only the second value takes effect — this is clearly a merge artifact and makes the expected balances (875 at line 1767, 900 at line 2124) hard to reason about. Remove one of the two calls in each test and confirm the intended dispute_fee for each scenario.
- client.configure_dispute_system(&admin, &arbitrator, &50i128);
client.configure_dispute_system(&admin, &arbitrator, &25i128);Also applies to: 1801-1802, 2107-2108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1751 - 1752, Tests call
configure_dispute_system twice in a row (configure_dispute_system(&admin,
&arbitrator, &50i128); then configure_dispute_system(&admin, &arbitrator,
&25i128); or 50 then 0), so the second call overrides the first; remove the
unintended duplicate call so only the intended dispute_fee is set (decide
whether the test should use 50, 25, or 0), update the test to call
configure_dispute_system only once (referencing the
configure_dispute_system(...) invocation), and then verify the test assertions
that depend on the fee (expected balances like 875 and 900) match the retained
dispute_fee; apply the same change for the other duplicate occurrences of
configure_dispute_system in the file.

Implementation Summary:
New Storage Symbols Added:
** FEE_ADMINS (Vec): Stores the list of addresses authorized to manage and modify fee parameters.
** FEE_THRESHOLD (u32): Stores the minimum number of multi-sig signers required to make fee modifications.
** configure_fee_multisig Introduced:
Multi-Sig Logic Integrated into configure_fees:
Instead of a single admin parameter, configure_fees now takes an admins: Vec
array.The contract dynamically iterates through the provided admins list to check if they belong to FEE_ADMINS.
On each matched valid admin, admin.require_auth() is called which enforces multi-signature confirmation natively using the Soroban auth framework.
Included robust safeguards against duplicate signatures/replay within the exact same configuration payload.
For backwards compatibility prior to multi-sig setup, it gracefully defaults to requiring the master CONTRACT_ADMIN.
Tests & Compilation:
The smart contract feature meets all required design patterns utilizing Soroban best practices!
Closes #515
Summary by CodeRabbit
Release Notes
New Features
Refactor