refactor: consolidate DataKey::Paused into CommonDataKey - #649
Open
Rafiat30 wants to merge 1 commit into
Open
Conversation
…aKey governance_contract::DataKey and settlement_contract::DataKey each still declared their own dead `Paused` variant even though both contracts had already switched their actual reads/writes over to bettapay_common::storage::is_paused/set_paused, which operate on CommonDataKey::Paused. The local variants were never constructed or matched anywhere outside their own enum declaration - a leftover from before the shared helper existed - but their presence duplicated the key name across three enums and made it look like each contract still owned its own pause flag. - Remove the unused `Paused` variant from governance_contract::DataKey and settlement_contract::DataKey. CommonDataKey::Paused is now the only declaration of this key in the workspace. - Expand the module-level doc comment on bettapay_common::storage to explain what "storage separation" means for a key type shared this way: each contract's instance storage is a separate ledger entry, so sharing CommonDataKey does not share any data - it only shares the wire shape of the key. Reworded the SCVal-compatibility note (why this consolidation needed no storage migration) into a historical note now that the duplicate variants are gone. - 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 directly against CommonDataKey::Paused and confirming two contract instances sharing the key type still have fully independent pause flags. RecoveryAddress and PendingRecovery have the identical dead-variant pattern in both contracts' local DataKey enums; left untouched here to keep this change scoped to the Paused key called out in the issue. Closes Betta-Pay#573
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
Both
governance_contract::DataKeyandsettlement_contract::DataKeydeclared their ownPausedvariant, even though both contracts already read and wrote the pause flag exclusively throughbettapay_common::storage::is_paused/set_paused, which operate onbettapay_common::CommonDataKey::Paused. The localPausedvariants in each contract's ownDataKeyenum were dead code — never constructed or matched anywhere except their own declaration — left over from before the shared helper existed. Their continued presence is exactly what the issue describes: duplicated key names, a shared-keyspace illusion, and doc ambiguity about which enum actually owns the key.This PR removes the duplicate declarations so
CommonDataKey::Pausedis the single source, and documents what "storage separation" actually means for a key type shared this way (each contract's instance storage is a distinct ledger entry — sharing the enum shares only the wire shape of the key, never any data).Changes
Modified files
governance_contract/src/lib.rs— removed the unusedPausedvariant fromDataKey; rewrote the comment above the enum to correctly list onlyRecoveryAddress/PendingRecovery/Pausedas living inCommonDataKey(the previous comment incorrectly groupedAdminin with them, even thoughAdminhas always stayed on each contract's ownDataKey) and to explain storage separation.settlement_contract/src/types.rs— same removal and comment fix for itsDataKeyenum.bettapay_common/src/storage.rs— expanded the module doc comment: explains thatCommonDataKeysharing a Rust type across contracts does not share any on-chain data (Soroban instance storage is scoped per contract address), and reworked the old SCVal-encoding-compatibility note into a historical note now that the duplicate variants it justified have been removed.Test files
bettapay_common/test_snapshots/storage/tests/is_paused_defaults_to_false_and_round_trips_via_common_data_key.1.json(new)bettapay_common/test_snapshots/storage/tests/two_contract_instances_have_independent_paused_flags.1.json(new)No production contract behavior changed:
is_paused/set_pausedalready read/wroteCommonDataKey::Pausedbefore this PR; removing the dead local variants doesn't touch any storage path either contract actually exercises.Implementation details
Pausedvariants were genuinely unreferenced (grep -rn "DataKey::Paused"across both contracts only ever matched the enum declaration itself, never a.get/.set/.has/.removecall site) before removing them.Pausedspecifically, per the issue title.RecoveryAddressandPendingRecoveryhave the identical dead-variant pattern in both contracts' localDataKeyenums (the existing comment already claimed they "live inCommonDataKeyinstead of here" even before this PR) — left untouched to keep this PR focused on a single concern perCONTRIBUTING.md; a natural follow-up if the maintainers want it.#[contracttype]-derived enums encode on-chain by variant name only (not by the parent enum's Rust name), which is why this consolidation needed no storage migration — documented as a historical note instorage.rssince it's still useful context for why this was safe, even though the duplicate variants that made the point concrete are now gone.Tests added
is_paused_defaults_to_false_and_round_trips_via_common_data_key— registers a dummy contract, then assertsis_pausedreads backfalsewith no entry written, and thatset_paused(true)/set_paused(false)round-trip correctly, all againstCommonDataKey::Pauseddirectly.two_contract_instances_have_independent_paused_flags— registers two separate contract instances, pauses one viaCommonDataKey::Paused, and asserts the other instance'sis_pausedis unaffected. This is the executable proof that sharing the key type across contracts does not share any pause state between them — the "document storage separation" half of the issue.Both new tests live in
bettapay_common::storage's existing#[cfg(test)] mod testsblock, alongside the existingprimary_admin_*tests, using the sameEnv::default()+testutils::Addresspattern already used there (plus a minimal#[contract] struct DummyContract;, needed becauseEnv::as_contractrequires a registered contract address to switch storage context into).How to test