Skip to content

Commit ba6ef21

Browse files
Chucks1093Chucks1093
authored
Merge pull request #563 from Unclebaffa/feat/staked-keys-regression-test
Feat/staked keys regression test
2 parents 8ccb706 + 5be3e6b commit ba6ef21

6 files changed

Lines changed: 969 additions & 70 deletions

File tree

‎STAKING_IMPLEMENTATION.md‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
# Key Staking Implementation Summary
2+
3+
## Overview
4+
This implementation adds key staking functionality to the creator-keys contract, ensuring that staked keys cannot be sold until they are explicitly unstaked by the holder.
5+
6+
## Changes Made
7+
8+
### 1. Core Contract Changes (`creator-keys/src/lib.rs`)
9+
10+
#### Data Storage
11+
- **Added `StakedBalance(Address, Address)` to `DataKey` enum**: Tracks staked amount per (creator, holder) pair
12+
- **Added `staked_balance()` helper function**: Returns storage key for staked balance lookup
13+
14+
#### New Public Functions
15+
16+
##### `stake_keys(env, creator, holder, amount) -> Result<(), ContractError>`
17+
- Stakes a specified amount of keys for a holder
18+
- Requires holder authorization
19+
- Validates that holder has sufficient liquid balance before staking
20+
- Increments the staked balance
21+
- **Errors**:
22+
- `NotPositiveAmount` if amount is zero
23+
- `InsufficientBalance` if liquid balance < amount
24+
- `ProtocolPaused` if contract is paused
25+
26+
##### `unstake_keys(env, creator, holder, amount) -> Result<(), ContractError>`
27+
- Unstakes a specified amount of previously staked keys
28+
- Requires holder authorization
29+
- Decrements the staked balance
30+
- Removes storage entry when staked balance reaches zero
31+
- **Errors**:
32+
- `NotPositiveAmount` if amount is zero
33+
- `InsufficientBalance` if staked balance < amount
34+
- `ProtocolPaused` if contract is paused
35+
36+
##### `get_staked_balance(env, creator, holder) -> u32`
37+
- Read-only view function
38+
- Returns the number of staked keys for a holder
39+
- Returns 0 if no keys are staked
40+
41+
##### `get_liquid_balance(env, creator, holder) -> u32`
42+
- Read-only view function
43+
- Returns sellable balance (total balance - staked balance)
44+
- Returns 0 if all keys are staked or holder has no keys
45+
46+
#### Modified Functions
47+
48+
##### `sell_key(env, creator, seller, min_proceeds) -> Result<u32, ContractError>`
49+
- **Modified to check liquid balance** instead of just total balance
50+
- Calculates liquid balance as: `total_balance - staked_balance`
51+
- Rejects sell attempts if liquid balance is zero
52+
- **Key Change**: Added staked balance check before processing sell
53+
54+
```rust
55+
// Check liquid balance (total balance - staked balance)
56+
let staked_balance_key = constants::storage::staked_balance(&creator, &seller);
57+
let staked_balance: u32 = env.storage().persistent().get(&staked_balance_key).unwrap_or(0);
58+
let liquid_balance = current_balance.saturating_sub(staked_balance);
59+
60+
if liquid_balance == 0 {
61+
return Err(ContractError::InsufficientBalance);
62+
}
63+
```
64+
65+
### 2. Test Suite (`creator-keys/tests/sell_requires_liquid_balance.rs`)
66+
67+
#### Test Cases
68+
69+
##### `test_sell_reverts_when_attempting_to_use_staked_keys`
70+
- **Setup**: Holder has 10 keys, stakes 6 (leaving 4 liquid)
71+
- **Action**: Attempt to sell 5 keys
72+
- **Expected**: Reverts with `InsufficientBalance` error
73+
- **Verifies**: Staked keys cannot be accessed for selling
74+
75+
##### `test_sell_succeeds_within_liquid_balance_limit`
76+
- **Setup**: Holder has 10 keys, stakes 6 (leaving 4 liquid)
77+
- **Action**: Sell exactly 4 keys (one at a time)
78+
- **Expected**: All 4 sells succeed
79+
- **Verifies**:
80+
- Liquid balance reaches 0
81+
- Staked balance unchanged at 6
82+
- Total balance is 6 (all staked)
83+
84+
##### `test_staked_balance_unchanged_after_sell_attempts`
85+
- **Setup**: Holder has 10 keys, stakes 6
86+
- **Action**:
87+
1. Attempt to sell 5 keys (fails)
88+
2. Successfully sell 4 keys
89+
- **Expected**: Staked balance remains at 6 throughout
90+
- **Verifies**: Staked balance is immutable through sell operations
91+
92+
## Acceptance Criteria
93+
94+
✅ **Sell of 5 reverts when only 4 liquid keys available**
95+
- Implemented in `test_sell_reverts_when_attempting_to_use_staked_keys`
96+
- When 10 total keys with 6 staked (4 liquid), selling 5 returns `InsufficientBalance`
97+
98+
✅ **Sell of 4 succeeds using only liquid balance**
99+
- Implemented in `test_sell_succeeds_within_liquid_balance_limit`
100+
- All 4 liquid keys can be sold individually
101+
- Staked keys remain untouched
102+
103+
✅ **Staked balance unchanged after both attempts**
104+
- Verified in both test cases
105+
- Failed sell attempt doesn't affect staked balance
106+
- Successful sells only reduce liquid balance
107+
- Staked balance remains constant at 6
108+
109+
## Implementation Details
110+
111+
### Storage Pattern
112+
- Staked balance is stored separately from total balance
113+
- Uses sparse storage (only stores non-zero values)
114+
- Storage key: `DataKey::StakedBalance(creator.clone(), holder.clone())`
115+
116+
### Balance Calculation
117+
- **Total Balance**: Stored in `KeyBalance(creator, holder)`
118+
- **Staked Balance**: Stored in `StakedBalance(creator, holder)`
119+
- **Liquid Balance**: Calculated as `total - staked` (uses `saturating_sub` for safety)
120+
121+
### Error Handling
122+
- Reuses existing `ContractError` variants:
123+
- `NotPositiveAmount`: For zero amount operations
124+
- `InsufficientBalance`: For insufficient liquid/staked balance
125+
- `ProtocolPaused`: For operations during pause
126+
- `Overflow`: For arithmetic overflow protection
127+
128+
### Authorization
129+
- Both `stake_keys` and `unstake_keys` require holder authorization
130+
- Uses `holder.require_auth()` to ensure only the holder can stake/unstake their keys
131+
132+
## Commit Structure
133+
134+
### Commit 1: Implementation
135+
```
136+
feat: implement key staking to prevent selling of staked keys
137+
138+
- Add StakedBalance data key to track staked keys per (creator, holder)
139+
- Add staked_balance storage helper function
140+
- Implement stake_keys() to lock keys from being sold
141+
- Implement unstake_keys() to unlock previously staked keys
142+
- Implement get_staked_balance() to query staked amount
143+
- Implement get_liquid_balance() to query sellable amount
144+
- Modify sell_key() to check liquid balance (total - staked) instead of just total balance
145+
```
146+
147+
### Commit 2: Tests
148+
```
149+
test: add tests for staked keys sell protection
150+
151+
- Test that selling 5 keys fails when only 4 liquid keys available (6 staked out of 10 total)
152+
- Test that selling exactly 4 liquid keys succeeds
153+
- Test that staked balance remains unchanged after failed and successful sell attempts
154+
- Verify InsufficientBalance error when attempting to sell more than liquid balance
155+
```
156+
157+
## Build Verification
158+
159+
⚠️ **Note**: Build and test execution could not be completed due to missing MSVC linker on the Windows build environment. However:
160+
- Code follows existing patterns from the codebase
161+
- Uses consistent error handling with other functions
162+
- Follows Rust and Soroban SDK best practices
163+
- Test structure matches existing test patterns
164+
165+
## Next Steps
166+
167+
To verify this implementation:
168+
1. Install MSVC Build Tools or Visual Studio with C++ support
169+
2. Run `cargo test --test sell_requires_liquid_balance`
170+
3. Run `cargo test` to ensure no regressions in existing tests
171+
4. Review contract size and gas costs if needed
172+
173+
## Security Considerations
174+
175+
- **No reentrancy risks**: All state changes happen atomically
176+
- **Overflow protection**: Uses checked arithmetic operations
177+
- **Authorization**: Requires holder auth for stake/unstake operations
178+
- **Sparse storage**: Only stores non-zero staked balances to save space
179+
- **Backward compatible**: Existing functionality unaffected (zero staked balance = all keys liquid)

‎TEST_FIX_SUMMARY.md‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
# Test Fix Summary
2+
3+
## Issue Identified
4+
5+
The initial tests had incorrect expectations about how `sell_key` behaves:
6+
7+
### Problem
8+
- `sell_key` sells **ONE key at a time** (not a batch)
9+
- Original tests expected the **first** sell to fail when holder has 4 liquid keys
10+
- This was incorrect - the first 4 sells should succeed, and the 5th should fail
11+
12+
### Root Cause of Test Failures
13+
14+
**Test 1: `test_sell_reverts_when_attempting_to_use_staked_keys`**
15+
- Expected: First `sell_key` call to fail with `InsufficientBalance`
16+
- Actual: First `sell_key` call succeeded (returned `Ok(9)` for new supply)
17+
- **Why**: With 4 liquid keys available, selling 1 key should succeed
18+
19+
**Test 2: `test_staked_balance_unchanged_after_sell_attempts`**
20+
- Expected: First `sell_key` to fail, then 4 more to succeed
21+
- Actual: Tried to sell 5 keys total, which exceeded liquid balance
22+
- **Why**: The test logic was backwards
23+
24+
## Solution Applied
25+
26+
### Fixed Test 1: `test_sell_reverts_when_attempting_to_use_staked_keys`
27+
28+
**Before:**
29+
```rust
30+
// Just tried to sell once and expected it to fail
31+
let result = client.try_sell_key(&creator, &holder, &None);
32+
assert_eq!(result, Err(Ok(ContractError::InsufficientBalance)));
33+
```
34+
35+
**After:**
36+
```rust
37+
// Sell 4 liquid keys successfully (one at a time)
38+
for _ in 0..4 {
39+
let result = client.try_sell_key(&creator, &holder, &None);
40+
assert!(result.is_ok(), "Selling within liquid balance should succeed");
41+
}
42+
43+
// Attempt to sell 5th key - should fail because only 4 were liquid
44+
let result = client.try_sell_key(&creator, &holder, &None);
45+
assert_eq!(
46+
result,
47+
Err(Ok(ContractError::InsufficientBalance)),
48+
"Selling more than liquid balance should fail"
49+
);
50+
```
51+
52+
### Fixed Test 2: `test_staked_balance_unchanged_after_sell_attempts`
53+
54+
**Before:**
55+
```rust
56+
let _ = client.try_sell_key(&creator, &holder, &None); // Unclear intent
57+
assert_eq!(client.get_staked_balance(&creator, &holder), 6);
58+
59+
for _ in 0..4 {
60+
client.sell_key(&creator, &holder, &None); // Would fail on 5th total
61+
}
62+
```
63+
64+
**After:**
65+
```rust
66+
// Successfully sell 4 keys (one at a time)
67+
for _ in 0..4 {
68+
client.sell_key(&creator, &holder, &None);
69+
}
70+
71+
// Verify staked balance unchanged after successful sells
72+
assert_eq!(client.get_staked_balance(&creator, &holder), 6);
73+
assert_eq!(client.get_liquid_balance(&creator, &holder), 0);
74+
75+
// Attempt to sell when no liquid balance remains (should fail)
76+
let result = client.try_sell_key(&creator, &holder, &None);
77+
assert_eq!(result, Err(Ok(ContractError::InsufficientBalance)));
78+
```
79+
80+
## Test Behavior Now Correctly Verifies
81+
82+
### Scenario: 10 total keys, 6 staked, 4 liquid
83+
84+
| Action | Liquid Before | Expected Result | Liquid After | Staked |
85+
|--------|--------------|-----------------|--------------|--------|
86+
| Sell #1 | 4 | ✅ Success | 3 | 6 |
87+
| Sell #2 | 3 | ✅ Success | 2 | 6 |
88+
| Sell #3 | 2 | ✅ Success | 1 | 6 |
89+
| Sell #4 | 1 | ✅ Success | 0 | 6 |
90+
| Sell #5 | 0 | ❌ Fail (InsufficientBalance) | 0 | 6 |
91+
92+
## Acceptance Criteria Verification
93+
94+
✅ **Sell of 5 (total) reverts when only 4 liquid keys available**
95+
- First 4 sells succeed
96+
- 5th sell fails with `InsufficientBalance`
97+
98+
✅ **Sell of 4 succeeds using only liquid balance**
99+
- All 4 liquid keys can be sold one at a time
100+
- Staked keys remain untouched
101+
102+
✅ **Staked balance unchanged after both attempts**
103+
- Remains at 6 after successful sells
104+
- Remains at 6 after failed sell attempt
105+
- Liquid balance correctly reaches 0 after 4 sells
106+
107+
## Implementation Correctness
108+
109+
The `sell_key` implementation is **correct**:
110+
111+
```rust
112+
// Check liquid balance (total balance - staked balance)
113+
let staked_balance_key = constants::storage::staked_balance(&creator, &seller);
114+
let staked_balance: u32 = env
115+
.storage()
116+
.persistent()
117+
.get(&staked_balance_key)
118+
.unwrap_or(0);
119+
let liquid_balance = current_balance.saturating_sub(staked_balance);
120+
121+
if liquid_balance == 0 {
122+
return Err(ContractError::InsufficientBalance);
123+
}
124+
```
125+
126+
This properly:
127+
1. Calculates liquid balance as `total - staked`
128+
2. Rejects sells when liquid balance is 0
129+
3. Allows sells when liquid balance > 0 (for the single key being sold)
130+
131+
## Commit
132+
133+
```
134+
18d975b fix: correct test expectations for sell_key liquid balance validation
135+
```
136+
137+
## Summary
138+
139+
The implementation was correct all along. The tests had incorrect expectations about the behavior of `sell_key` which sells one key per call, not a batch. Tests now correctly verify that:
140+
141+
1. Multiple sells within liquid balance succeed
142+
2. Sells beyond liquid balance fail
143+
3. Staked balance remains unchanged throughout

0 commit comments

Comments
 (0)