Skip to content

Commit dc0e97a

Browse files
eulamieulami
committed
fix(onchain): enforce question progression and retirement indexes
1 parent 8c60edf commit dc0e97a

5 files changed

Lines changed: 206 additions & 49 deletions

File tree

‎onchain/contracts/stellar_hunts/src/lib.rs‎

Lines changed: 73 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,10 @@ pub struct LevelProgress {
4242
pub level: Levels,
4343
// u8 is not a valid Soroban Val in soroban-sdk 22 — the smallest
4444
// native unsigned integer is `u32`.
45+
/// Index of the next question in `QuestionsByLevel` that the player must
46+
/// answer. Retiring or moving a question compacts that index; because
47+
/// progress records are not enumerable, those administrative changes may
48+
/// invalidate an existing cursor, which must then be reset by the player.
4549
pub last_question_index: u32,
4650
pub is_completed: bool,
4751
pub attempts: u32,
@@ -111,6 +115,7 @@ pub enum Error {
111115
LevelImmutable = 11,
112116
ArithmeticOverflow = 12,
113117
ContractPaused = 13,
118+
WrongQuestion = 14,
114119
}
115120

116121
// ---------------------------------------------------------------------
@@ -266,7 +271,11 @@ impl StellarHunts {
266271

267272
let old_level = existing.level.clone();
268273

269-
if old_level != level {
274+
let is_retired = env
275+
.storage()
276+
.persistent()
277+
.has(&DataKey::RetiredQuestion(question_id));
278+
if old_level != level && !is_retired {
270279
let per_level: u32 = env
271280
.storage()
272281
.instance()
@@ -304,39 +313,7 @@ impl StellarHunts {
304313
&new_next_index,
305314
);
306315

307-
// `old_idx` counts the questions in the old level, so it is
308-
// >= 1 whenever the question being moved exists there; a
309-
// checked_sub keeps the underflow behavior explicit anyway.
310-
let last_old_idx = old_idx
311-
.checked_sub(1)
312-
.unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow));
313-
for i in 0..old_idx {
314-
let qid: u64 = env
315-
.storage()
316-
.persistent()
317-
.get(&DataKey::QuestionsByLevel(old_level.clone(), i))
318-
.unwrap_or(0u64);
319-
if qid == question_id {
320-
for j in i..last_old_idx {
321-
let next_qid: u64 = env
322-
.storage()
323-
.persistent()
324-
.get(&DataKey::QuestionsByLevel(old_level.clone(), j + 1))
325-
.unwrap_or(0u64);
326-
env.storage()
327-
.persistent()
328-
.set(&DataKey::QuestionsByLevel(old_level.clone(), j), &next_qid);
329-
}
330-
env.storage()
331-
.persistent()
332-
.remove(&DataKey::QuestionsByLevel(old_level.clone(), last_old_idx));
333-
break;
334-
}
335-
}
336-
env.storage().persistent().set(
337-
&DataKey::QuestionPerLevelIndex(old_level.clone()),
338-
&last_old_idx,
339-
);
316+
remove_question_from_level(&env, old_level.clone(), question_id);
340317
}
341318

342319
let hashed: BytesN<32> = env.crypto().sha256(&answer).into();
@@ -372,9 +349,9 @@ impl StellarHunts {
372349
if !env.storage().persistent().has(&key) {
373350
panic_with_error!(&env, Error::QuestionNotFound);
374351
}
375-
env.storage()
376-
.persistent()
377-
.set(&DataKey::RetiredQuestion(question_id), &true);
352+
let question: Question = env.storage().persistent().get(&key).unwrap();
353+
remove_question_from_level(&env, question.level.clone(), question_id);
354+
env.storage().persistent().set(&DataKey::RetiredQuestion(question_id), &true);
378355
env.events()
379356
.publish((Symbol::new(&env, "question_retired"),), (question_id,));
380357
}
@@ -432,6 +409,21 @@ impl StellarHunts {
432409
last_attempt_ledger: 0,
433410
});
434411

412+
if lp.last_question_index == u32::MAX {
413+
panic_with_error!(&env, Error::ArithmeticOverflow);
414+
}
415+
416+
let expected_question_id: u64 = env
417+
.storage()
418+
.persistent()
419+
.get(&DataKey::QuestionsByLevel(question.level.clone(), lp.last_question_index))
420+
.unwrap_or(0u64);
421+
if expected_question_id != question_id
422+
|| env.storage().persistent().has(&DataKey::RetiredQuestion(question_id))
423+
{
424+
panic_with_error!(&env, Error::WrongQuestion);
425+
}
426+
435427
let current_ledger = env.ledger().sequence();
436428
if lp.last_attempt_ledger == current_ledger {
437429
panic_with_error!(&env, Error::AttemptTooSoon);
@@ -450,12 +442,12 @@ impl StellarHunts {
450442
.last_question_index
451443
.checked_add(1)
452444
.unwrap_or_else(|| panic_with_error!(&env, Error::ArithmeticOverflow));
453-
let per_level: u32 = env
445+
let active_questions: u32 = env
454446
.storage()
455-
.instance()
456-
.get(&DataKey::QuestionPerLevel)
457-
.unwrap_or(5u32);
458-
if lp.last_question_index >= per_level {
447+
.persistent()
448+
.get(&DataKey::QuestionPerLevelIndex(question.level.clone()))
449+
.unwrap_or(0u32);
450+
if lp.last_question_index >= active_questions {
459451
lp.is_completed = true;
460452
let next = question.level.next();
461453
let pp = PlayerProgress {
@@ -729,6 +721,45 @@ impl StellarHunts {
729721
}
730722
}
731723

724+
/// Removes a question from its level index and compacts subsequent entries.
725+
/// Returns the new number of questions in that level.
726+
fn remove_question_from_level(env: &Env, level: Levels, question_id: u64) -> u32 {
727+
let count: u32 = env
728+
.storage()
729+
.persistent()
730+
.get(&DataKey::QuestionPerLevelIndex(level.clone()))
731+
.unwrap_or(0u32);
732+
if count == 0 {
733+
return 0;
734+
}
735+
let Some(index) = (0..count).find(|index| {
736+
env.storage()
737+
.persistent()
738+
.get::<DataKey, u64>(&DataKey::QuestionsByLevel(level.clone(), *index))
739+
== Some(question_id)
740+
}) else {
741+
return count;
742+
};
743+
for current in index..count - 1 {
744+
let next: u64 = env
745+
.storage()
746+
.persistent()
747+
.get(&DataKey::QuestionsByLevel(level.clone(), current + 1))
748+
.unwrap_or(0u64);
749+
env.storage()
750+
.persistent()
751+
.set(&DataKey::QuestionsByLevel(level.clone(), current), &next);
752+
}
753+
let new_count = count - 1;
754+
env.storage()
755+
.persistent()
756+
.remove(&DataKey::QuestionsByLevel(level.clone(), new_count));
757+
env.storage()
758+
.persistent()
759+
.set(&DataKey::QuestionPerLevelIndex(level), &new_count);
760+
new_count
761+
}
762+
732763
// ---------------------------------------------------------------------
733764
// Internal helpers
734765
// ---------------------------------------------------------------------

‎onchain/contracts/stellar_hunts/src/test.rs‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,76 @@ fn test_submit_answer_incorrect_does_not_progress() {
272272
assert_eq!(new_level, crate::Levels::Easy);
273273
}
274274

275+
#[test]
276+
fn test_submit_answer_requires_next_indexed_question() {
277+
let env = Env::default();
278+
env.mock_all_auths();
279+
env.ledger().set_sequence_number(100_000);
280+
let (_admin, _contract_id, client) = init_with_admin(&env);
281+
let player = user(&env);
282+
let level = crate::Levels::Easy;
283+
client.set_question_per_level(&2u32);
284+
client.add_question(&level, &b(&env, "Q1"), &b(&env, "A1"), &b(&env, "H1"));
285+
client.add_question(&level, &b(&env, "Q2"), &b(&env, "A2"), &b(&env, "H2"));
286+
287+
let out_of_order = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
288+
client.submit_answer(&player, &2u64, &b(&env, "A2"));
289+
}));
290+
assert!(out_of_order.is_err());
291+
assert!(panic_text(&out_of_order).contains("Error(Contract, #14)"));
292+
assert_eq!(client.get_player_level_progress(&player, &level).last_question_index, 0);
293+
294+
assert!(client.submit_answer(&player, &1u64, &b(&env, "A1")));
295+
env.ledger().set_sequence_number(env.ledger().sequence() + 1);
296+
let duplicate = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
297+
client.submit_answer(&player, &1u64, &b(&env, "A1"));
298+
}));
299+
assert!(duplicate.is_err());
300+
assert!(panic_text(&duplicate).contains("Error(Contract, #14)"));
301+
assert_eq!(client.get_player_level_progress(&player, &level).last_question_index, 1);
302+
assert_eq!(client.get_player_level(&player), level);
303+
304+
env.ledger().set_sequence_number(env.ledger().sequence() + 1);
305+
assert!(client.submit_answer(&player, &2u64, &b(&env, "A2")));
306+
assert_eq!(client.get_player_level(&player), crate::Levels::Medium);
307+
}
308+
309+
#[test]
310+
fn test_retire_question_compacts_level_index_and_answer_order() {
311+
let env = Env::default();
312+
env.mock_all_auths();
313+
env.ledger().set_sequence_number(100_000);
314+
let (_admin, contract_id, client) = init_with_admin(&env);
315+
let player = user(&env);
316+
let level = crate::Levels::Easy;
317+
client.set_question_per_level(&3u32);
318+
client.add_question(&level, &b(&env, "Q1"), &b(&env, "A1"), &b(&env, "H1"));
319+
client.add_question(&level, &b(&env, "Q2"), &b(&env, "A2"), &b(&env, "H2"));
320+
client.add_question(&level, &b(&env, "Q3"), &b(&env, "A3"), &b(&env, "H3"));
321+
322+
client.retire_question(&1u64);
323+
assert_eq!(client.get_question_in_level(&level, &0u32), b(&env, "Q2"));
324+
assert_eq!(client.get_question_in_level(&level, &1u32), b(&env, "Q3"));
325+
let count: u32 = env.as_contract(&contract_id, || {
326+
env.storage()
327+
.persistent()
328+
.get(&crate::DataKey::QuestionPerLevelIndex(level.clone()))
329+
.unwrap()
330+
});
331+
assert_eq!(count, 2);
332+
333+
// Compaction can invalidate stored player cursors; fresh progress starts
334+
// at the new first question and retired question IDs can no longer pass.
335+
let retired_answer = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
336+
client.submit_answer(&player, &1u64, &b(&env, "A1"));
337+
}));
338+
assert!(retired_answer.is_err());
339+
assert!(client.submit_answer(&player, &2u64, &b(&env, "A2")));
340+
env.ledger().set_sequence_number(env.ledger().sequence() + 1);
341+
assert!(client.submit_answer(&player, &3u64, &b(&env, "A3")));
342+
assert_eq!(client.get_player_level(&player), crate::Levels::Medium);
343+
}
344+
275345
// ---------------------------------------------------------------------
276346
// Hint request after answering a question
277347
// ---------------------------------------------------------------------

‎onchain/contracts/stellar_hunts_nft/src/lib.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ impl StellarHuntsNft {
239239
.storage()
240240
.instance()
241241
.get(&NftDataKey::Admin)
242-
.expect("admin not set");
242+
.unwrap_or_else(|| panic_with_error!(&env, Error::NotInitialized));
243243
admin.require_auth();
244244

245245
env.storage()
@@ -252,7 +252,7 @@ impl StellarHuntsNft {
252252
.storage()
253253
.instance()
254254
.get(&NftDataKey::Admin)
255-
.expect("admin not set");
255+
.unwrap_or_else(|| panic_with_error!(&env, Error::NotInitialized));
256256
admin.require_auth();
257257

258258
env.storage()

‎onchain/contracts/stellar_hunts_nft/src/test.rs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,63 @@ fn test_admin_handover_preserves_minter_roles() {
6262
assert!(client.has_minter_role(&new_minter));
6363
}
6464

65+
#[test]
66+
#[should_panic(expected = "Error(Contract, #6)")]
67+
fn test_grant_minter_role_uninitialized_returns_contract_error() {
68+
let env = Env::default();
69+
env.mock_all_auths();
70+
let contract_id = env.register_contract(None, StellarHuntsNft);
71+
StellarHuntsNftClient::new(&env, &contract_id).grant_minter_role(&recipient(&env));
72+
}
73+
74+
#[test]
75+
#[should_panic(expected = "Error(Contract, #6)")]
76+
fn test_revoke_minter_role_uninitialized_returns_contract_error() {
77+
let env = Env::default();
78+
env.mock_all_auths();
79+
let contract_id = env.register_contract(None, StellarHuntsNft);
80+
StellarHuntsNftClient::new(&env, &contract_id).revoke_minter_role(&recipient(&env));
81+
}
82+
83+
#[test]
84+
#[should_panic(expected = "Error(Contract, #7)")]
85+
fn test_paused_mint_returns_contract_paused_code_7() {
86+
let env = Env::default();
87+
env.mock_all_auths();
88+
let admin = admin(&env);
89+
let game = recipient(&env);
90+
let contract_id = env.register_contract(None, StellarHuntsNft);
91+
let client = StellarHuntsNftClient::new(&env, &contract_id);
92+
client.init(
93+
&admin,
94+
&game,
95+
&String::from_str(&env, "ipfs://placeholder/"),
96+
&String::from_str(&env, "StellarHuntsBadge"),
97+
&String::from_str(&env, "SHB"),
98+
);
99+
client.pause();
100+
client.mint_level_badge(&game, &recipient(&env), &crate::Levels::Easy);
101+
}
102+
103+
#[test]
104+
#[should_panic(expected = "Error(Contract, #1)")]
105+
fn test_unregistered_minter_returns_not_authorized_code_1() {
106+
let env = Env::default();
107+
env.mock_all_auths();
108+
let admin = admin(&env);
109+
let game = recipient(&env);
110+
let contract_id = env.register_contract(None, StellarHuntsNft);
111+
let client = StellarHuntsNftClient::new(&env, &contract_id);
112+
client.init(
113+
&admin,
114+
&game,
115+
&String::from_str(&env, "ipfs://placeholder/"),
116+
&String::from_str(&env, "StellarHuntsBadge"),
117+
&String::from_str(&env, "SHB"),
118+
);
119+
client.mint_level_badge(&recipient(&env), &recipient(&env), &crate::Levels::Easy);
120+
}
121+
65122
#[test]
66123
fn test_mint_via_game_contract_then_query() {
67124
let env = Env::default();

‎onchain/docs/error-codes.md‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@ This document lists all error codes raised by the StellarHunts Soroban contracts
1818
| 10 | AttemptTooSoon | `submit_answer` | The caller is attempting to submit another answer too quickly (rate limit). |
1919
| 11 | LevelImmutable | - | Reserved for future use (currently defined but not raised). |
2020
| 12 | ArithmeticOverflow | `add_question`, `submit_answer`, `update_question` | An arithmetic operation would overflow. |
21-
| 6 | ContractPaused | `claim_level_completion_nft`, `submit_answer` | The contract is paused and cannot accept submissions. |
21+
| 13 | ContractPaused | `claim_level_completion_nft`, `submit_answer` | The contract is paused and cannot accept submissions. |
22+
| 14 | WrongQuestion | `submit_answer` | The submitted question is not the next active question in the level index. |
2223

23-
**Note:** Code 6 is used for both `NotInitialized` and `ContractPaused`. This is a legacy duplication that should be avoided in new code.
2424

2525
## stellar_hunts_nft Error Codes
2626

@@ -32,9 +32,8 @@ This document lists all error codes raised by the StellarHunts Soroban contracts
3232
| 4 | InvalidBaseUri | `init` | The provided base URI for metadata is invalid. |
3333
| 5 | MetadataTooLarge | `init` | The metadata exceeds the maximum allowed size. |
3434
| 6 | NotInitialized | `mint_level_badge` | The contract is not initialized. |
35-
| 6 | ContractPaused | `mint_level_badge` | The contract is paused and cannot mint badges. |
35+
| 7 | ContractPaused | `mint_level_badge` | The contract is paused and cannot mint badges. |
3636

37-
**Note:** Code 6 is used for both `NotInitialized` and `ContractPaused` in the NFT contract as well. This duplication should be avoided in new code.
3837

3938
## Error Code Assignment Guidelines
4039

@@ -48,7 +47,7 @@ When adding new error codes:
4847

4948
## Reserved/Legacy Codes
5049

51-
- Code 6 in both contracts is currently used for two different errors (`NotInitialized` and `ContractPaused`). This is a legacy pattern that should not be repeated for new error codes.
50+
- Error codes are unique within each contract; the NFT contract's `ContractPaused` uses code 7.
5251

5352
## Off-Chain Client Integration
5453

0 commit comments

Comments
 (0)