docs: document CommonDataKey storage separation, cover pause key with tests - #677
Open
Rafiat30 wants to merge 1 commit into
Open
docs: document CommonDataKey storage separation, cover pause key with tests#677Rafiat30 wants to merge 1 commit into
Rafiat30 wants to merge 1 commit into
Conversation
The Paused/RecoveryAddress/PendingRecovery/Threshold dead-variant duplication between governance_contract::DataKey and settlement_contract::DataKey was already removed in a prior commit, so CommonDataKey::Paused is already the single declaration of the pause key in the workspace. What was missing was the "document storage separation" half of the issue and tests that exercise it directly: - Expand bettapay_common::storage's module doc comment with a "Storage separation" section explaining that sharing CommonDataKey between contracts shares only the key's wire shape, not any data - each contract's instance storage is its own ledger entry. - Fix a stale comment in governance_contract::DataKey and settlement_contract::DataKey that incorrectly implied Admin lived in CommonDataKey (it stays local, stored as a multisig Vec<Address>) and omitted Threshold from the list of keys that do. - Add is_paused_defaults_to_false_and_round_trips_via_common_data_key and two_contract_instances_have_independent_paused_flags to bettapay_common::storage's test module, exercising is_paused/ set_paused against CommonDataKey::Paused directly and confirming two contract instances sharing the key type still have independent pause flags. cargo test --workspace passes (190 tests). Closes Betta-Pay#573
|
@Rafiat30 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! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #573
Summary
The issue reported that
governance_contract::DataKeyandsettlement_contract::DataKeyeach declared their ownPausedvariant, duplicating the pause-key name across the workspace and inviting confusion about whether the two contracts shared a keyspace.The duplicate
Paused(andRecoveryAddress/PendingRecovery/Threshold) variants had already been removed from both contracts' localDataKeyenums in an earlier merged commit (refactor: deduplicate common data keys and use shared threshold), which routed all reads/writes throughbettapay_common::storage::is_paused/set_paused, operating onCommonDataKey::Paused. So as ofmain,CommonDataKey::Pausedis already the single declaration of the pause key in the workspace — verified with:What the issue's acceptance criteria still called for — "Document storage separation" — was not done, and there were no tests exercising the shared key directly. This PR closes that gap.
Changes
Modified files
bettapay_common/src/storage.rs— expanded the module-level doc comment with a new "Storage separation" section explaining precisely what sharingCommonDataKeydoes and doesn't mean: Soroban instance storage is one ledger entry per contract instance, sogovernance_contract'sCommonDataKey::Pausedandsettlement_contract'sCommonDataKey::Pausedare two independent booleans in two independent ledger entries.CommonDataKeyonly fixes the wire shape of the key, not the data. Reworded the SCVal-compatibility note into a historical note now that the duplicate variants are gone.bettapay_common/src/lib.rs— updated the crate-level module summary forstorageto match (drops a reference to aread_adminhelper that isn't part of this crate, lists the keysCommonDataKeyactually owns).governance_contract/src/lib.rsandsettlement_contract/src/types.rs— fixed a stale comment above each contract's localDataKeyenum. It previously read "Admin, RecoveryAddress, PendingRecovery, and Paused live inCommonDataKey", which is wrong:Adminstays local to each contract (stored as a multisigVec<Address>, whichCommonDataKeydoesn't own) andThreshold— which does live inCommonDataKey— was missing from the list. Corrected to "RecoveryAddress, PendingRecovery, Paused, and Threshold live inCommonDataKey... Admin stays local...".New files (test snapshots, auto-generated by the Soroban test harness)
bettapay_common/test_snapshots/storage/tests/is_paused_defaults_to_false_and_round_trips_via_common_data_key.1.jsonbettapay_common/test_snapshots/storage/tests/two_contract_instances_have_independent_paused_flags.1.jsonTest files
bettapay_common/src/storage.rs(#[cfg(test)] mod tests) — added:is_paused_defaults_to_false_and_round_trips_via_common_data_key: registers a dummy contract, confirmsis_pausedreads backfalsewhen nothing has been written yet (i.e. a missingCommonDataKey::Pausedentry is "unpaused", not a panic), then round-tripsset_paused(true)/set_paused(false)throughis_paused.two_contract_instances_have_independent_paused_flags: registers two separate dummy contract instances (standing in for a governance-like and a settlement-like contract), pauses one viaCommonDataKey::Paused, and asserts the other instance'sis_paused()is unaffected — this is the direct demonstration of "storage separation" called out in the issue.Both use a small
#[contract] struct DummyContract;test fixture, sinceEnv::as_contractneeds a real registered contract address to exercise instance-storage helpers.How to test
cargo test --workspaceAll 190 tests pass. To run just the new/relevant tests:
cargo test -p bettapay_common --lib storage::tests