Skip to content

refactor: consolidate DataKey::Paused into CommonDataKey #649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rafiat30 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: consolidate DataKey::Paused into CommonDataKey #649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 75 additions & 12 deletions bettapay_common/src/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,33 @@
//! Each contract keeps its own private `DataKey` enum for keys that are
//! contract-specific (e.g. settlement's `Merchant(Address)` or governance's
//! `Anchor(Address)`). The keys that are semantically shared — the pause
//! flag, the recovery address, and the pending recovery operation — live in
//! [`CommonDataKey`] so every contract reads and writes them in exactly the
//! same shape. The admin role is *not* one of these: both contracts store it
//! as a multisig `Vec<Address>` under their own `DataKey::Admin`, so there is
//! no single-`Address` shape for this crate to own.
//! flag, the recovery address, and the pending recovery operation — are
//! declared exactly once, in [`CommonDataKey`] below, and both contracts
//! import that type rather than redeclaring their own `Paused` /
//! `RecoveryAddress` / `PendingRecovery` variants. That single declaration
//! is what "storage separation" means here: `governance_contract` and
//! `settlement_contract` each have their own instance-storage ledger entry
//! (Soroban storage is scoped per contract instance), so sharing this enum
//! does not share any *data* between them — a settlement contract's
//! `CommonDataKey::Paused` and a governance contract's
//! `CommonDataKey::Paused` are two independent booleans in two independent
//! ledger entries. The type is shared purely so both contracts agree on the
//! wire shape of these keys; nothing here lets one contract read or write
//! the other's storage. The admin role is *not* one of these: both
//! contracts store it as a multisig `Vec<Address>` under their own
//! `DataKey::Admin`, so there is no single-`Address` shape for this crate
//! to own.
//!
//! The on-chain SCVal encoding of a Soroban `#[contracttype]` enum is based on
//! the variant name only; the parent enum's Rust name is not part of the
//! encoding. So a value written under `governance_contract::DataKey::Paused`
//! reads back identically through `bettapay_common::CommonDataKey::Paused`,
//! which is what allows both contracts to share this enum without disturbing
//! any existing storage entry.
//! Historical note: `Paused` (and `RecoveryAddress` / `PendingRecovery`)
//! used to be declared redundantly in each contract's own `DataKey` enum
//! as well as here. That was safe only because the on-chain SCVal encoding
//! of a Soroban `#[contracttype]` enum is based on the variant name alone —
//! the parent enum's Rust name is not part of the encoding — so a value
//! written under the old `governance_contract::DataKey::Paused` read back
//! identically through `CommonDataKey::Paused`. The duplicate variants have
//! since been removed from both contracts' `DataKey` enums; `CommonDataKey`
//! is now the single source for these keys, and no storage migration was
//! needed to get there.

use soroban_sdk::{contracttype, Address, Env, String, Vec};

Expand Down Expand Up @@ -126,7 +141,55 @@ pub fn bump_instance_ttl(env: &Env) {
#[cfg(test)]
mod tests {
use super::*;
use soroban_sdk::{testutils::Address as _, vec};
use soroban_sdk::{contract, testutils::Address as _, vec};

/// No-op contract used only to obtain a real, registered contract
/// address for [`Env::as_contract`] — instance storage helpers like
/// [`is_paused`]/[`set_paused`] can only be exercised inside a
/// registered contract's storage context.
#[contract]
struct DummyContract;

#[test]
fn is_paused_defaults_to_false_and_round_trips_via_common_data_key() {
let env = Env::default();
let contract_id = env.register_contract(None, DummyContract);
env.as_contract(&contract_id, || {
// No entry written yet — CommonDataKey::Paused is the *only*
// pause key either contract can read, so a missing entry must
// read back as unpaused rather than panicking.
assert!(!is_paused(&env));

set_paused(&env, true);
assert!(is_paused(&env));

set_paused(&env, false);
assert!(!is_paused(&env));
});
}

#[test]
fn two_contract_instances_have_independent_paused_flags() {
// Instance storage is scoped per contract address, so pausing one
// instance under CommonDataKey::Paused must not affect another
// instance that happens to share the same key type — this is what
// "storage separation" means despite governance_contract and
// settlement_contract both using CommonDataKey.
let env = Env::default();
let governance_like = env.register_contract(None, DummyContract);
let settlement_like = env.register_contract(None, DummyContract);

env.as_contract(&governance_like, || {
set_paused(&env, true);
});

env.as_contract(&settlement_like, || {
assert!(!is_paused(&env));
});
env.as_contract(&governance_like, || {
assert!(is_paused(&env));
});
}

#[test]
fn primary_admin_returns_first_entry() {
Expand Down
88 changes: 88 additions & 0 deletions ...hots/storage/tests/is_paused_defaults_to_false_and_round_trips_via_common_data_key.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
{
"generators": {
"address": 1,
"nonce": 0
},
"auth": [
[]
],
"ledger": {
"protocol_version": 21,
"sequence_number": 0,
"timestamp": 0,
"network_id": "0000000000000000000000000000000000000000000000000000000000000000",
"base_reserve": 0,
"min_persistent_entry_ttl": 4096,
"min_temp_entry_ttl": 16,
"max_entry_ttl": 6312000,
"ledger_entries": [
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent",
"val": {
"contract_instance": {
"executable": {
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
},
"storage": [
{
"key": {
"vec": [
{
"symbol": "Paused"
}
]
},
"val": {
"bool": false
}
}
]
}
}
}
},
"ext": "v0"
},
518400
]
],
[
{
"contract_code": {
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_code": {
"ext": "v0",
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"code": ""
}
},
"ext": "v0"
},
518400
]
]
]
},
"events": []
}
122 changes: 122 additions & 0 deletions .../test_snapshots/storage/tests/two_contract_instances_have_independent_paused_flags.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
{
"generators": {
"address": 2,
"nonce": 0
},
"auth": [
[],
[],
[]
],
"ledger": {
"protocol_version": 21,
"sequence_number": 0,
"timestamp": 0,
"network_id": "0000000000000000000000000000000000000000000000000000000000000000",
"base_reserve": 0,
"min_persistent_entry_ttl": 4096,
"min_temp_entry_ttl": 16,
"max_entry_ttl": 6312000,
"ledger_entries": [
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent",
"val": {
"contract_instance": {
"executable": {
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
},
"storage": [
{
"key": {
"vec": [
{
"symbol": "Paused"
}
]
},
"val": {
"bool": true
}
}
]
}
}
}
},
"ext": "v0"
},
518400
]
],
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": "ledger_key_contract_instance",
"durability": "persistent"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": "ledger_key_contract_instance",
"durability": "persistent",
"val": {
"contract_instance": {
"executable": {
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
},
"storage": null
}
}
}
},
"ext": "v0"
},
518400
]
],
[
{
"contract_code": {
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_code": {
"ext": "v0",
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"code": ""
}
},
"ext": "v0"
},
518400
]
]
]
},
"events": []
}
13 changes: 8 additions & 5 deletions governance_contract/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,16 @@ const SYSTEM_PARAM_TTL_BUMP: u32 = TTL_BUMP_LEDGERS;
const READ_INSTANCE_TTL_THRESHOLD: u32 = 50_000;
const READ_INSTANCE_TTL_BUMP: u32 = 100_000;

// Admin, RecoveryAddress, PendingRecovery, and Paused live in
// RecoveryAddress, PendingRecovery, and Paused live in
// `bettapay_common::storage::CommonDataKey` instead of here - see that
// type's doc comment for why a shared key type is safe to mix with this
// contract's own storage without a migration.
// contract's own storage without a migration. Admin is NOT one of these:
// both contracts keep it as their own `DataKey::Admin` below, since it's a
// multisig `Vec<Address>` rather than the single-`Address` shape
// `CommonDataKey` would need to own it. Storage stays fully separate
// otherwise - this contract's `DataKey` and `CommonDataKey` share no
// on-chain state, they simply both read/write the same instance-storage
// ledger entry via distinct key variants.
#[derive(Clone)]
#[contracttype]
enum DataKey {
Expand All @@ -230,9 +236,6 @@ enum DataKey {
/// Storage key for the anchor address associated with a specific asset.
Anchor(Address),

/// Storage key for the pause state flag.
Paused,

/// Storage key for a scheduled operation.
/// Uses persistent storage, keyed by operation hash, to store execution timestamp.
ScheduledOperation(BytesN<32>),
Expand Down
12 changes: 8 additions & 4 deletions settlement_contract/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,16 @@ pub struct FeeConfig {
pub network_fee_bps: u32,
}

// Admin, RecoveryAddress, PendingRecovery, and Paused live in
// RecoveryAddress, PendingRecovery, and Paused live in
// `bettapay_common::storage::CommonDataKey` instead of here - see that
// type's doc comment for why a shared key type is safe to mix with this
// contract's own storage without a migration.
// contract's own storage without a migration. Admin is NOT one of these:
// both contracts keep it as their own `DataKey::Admin` below, since it's a
// multisig `Vec<Address>` rather than the single-`Address` shape
// `CommonDataKey` would need to own it. Storage stays fully separate
// otherwise - this contract's `DataKey` and `CommonDataKey` share no
// on-chain state, they simply both read/write the same instance-storage
// ledger entry via distinct key variants.

#[derive(Clone)]
#[contracttype]
Expand Down Expand Up @@ -189,8 +195,6 @@ pub(crate) enum DataKey {
DefaultRule,
/// Persistent — one per payment, high volume.
Payment(BytesN<32>),
/// Instance — singleton boolean, read on every mutating call.
Paused,
/// Storage key for a scheduled operation.
ScheduledOperation(BytesN<32>),
}