feat: add contract deployment validation - #435
Open
Stanley-Owoh wants to merge 2 commits into
Open
Conversation
|
@Stanley-Owoh this PR currently has merge conflicts. Please resolve the conflicts before it can be merged automatically. |
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.
Add contract deployment parameter validation
Closes #285
Summary
Adds typed, fail-fast validation of deployment parameters at initialization
time for the payroll and payment_executor contracts. Mis-wired
deployments (duplicate dependency addresses, self-references, role/dependency
collisions) now fail at initialize with a typed DeploymentError instead of
silently producing a broken deployment that only surfaces at first payment.
Also introduces an optional, admin-gated, one-time network identifier on the
payroll contract for multi-network deployments (testnet/mainnet QA).
Changes
payroll contract (contracts/payroll/src/lib.rs)
New DeploymentError contract error enum:
AlreadyInitialized = 1
DuplicateDependency = 2
RoleConflictsWithDependency = 3
SelfReference = 4
NotInitialized = 5
NetworkIdAlreadySet = 6
InvalidNetworkId = 7
initialize(admin, token, verifier, commitment, treasury, treasury_owner)
now returns Result<(), DeploymentError> and validates before writing any
state:
no dependency may point at the payroll contract itself (SelfReference)
token / verifier / commitment must be pairwise distinct
(DuplicateDependency)
admin, treasury, and treasury_owner must not collide with a dependency
address (RoleConflictsWithDependency)
New one-time network identifier:
set_network_id(admin, network_id) — admin-gated, stores a non-empty
string of at most MAX_NETWORK_ID_LEN (128) bytes, emits a
network_id_set event; rejects repeats with NetworkIdAlreadySet and
over-long/empty values with InvalidNetworkId
get_network_id() -> Option — read accessor
payment_executor contract (contracts/payment_executor/src/lib.rs)
New DeploymentError enum (AlreadyInitialized = 1, DuplicateDependency = 2, SelfReference = 3)
initialize(addresses) now returns Result<(), DeploymentError> and
rejects self-references and duplicate dependency addresses before storing
them; the initial asset allowlisting behavior is unchanged
New test crate: tests/deployment
Dedicated integration-test crate (18 tests, all passing) covering:
success paths for both contracts' initialize
every DeploymentError variant for both contracts
privacy check: the initialization event exposes only public configuration
addresses — never salary amounts, commitments, or other private values
network-id lifecycle: set/get, admin gating, one-time semantics, length
limits, event emission
Test-suite repairs (pre-existing failures verified at HEAD)
These tests were failing on main but were previously masked by workspace
compile breakage:
payment_executor: stale event-count assertions updated for the
TreasuryAssetAllowedUpdated event (issue #175)
payment_executor: implemented the per-period payment_count increment in
execute_payment (invariant I-8 documented it; the field was never
incremented)
recovery_tests: two tests assumed partial batch state survives an errored
call — impossible under Soroban's atomic execution. Rewritten to assert
full rollback plus individual-payment recovery
treasury_authorization_tests: the mismatched-treasury test expected a
panic that the placeholder token (which intentionally omits
from.require_auth()) can never produce. Converted to a test pinning the
current behavior so swapping in a real SEP-41 token surfaces here
migration_tests: fixed fixture drift (zero ledger timestamp, registry
commitment not synced on rotation, hardcoded company ID, unclosed period
blocking new periods, migration reads of non-existent reduced-fixture
state)
integration_tests: fixed e2e event-topic assertion to handle both topic
layouts (payroll-domain events use ("payroll", ); other emitters
put the name first)
Lint debt cleanup so cargo clippy --workspace --all-targets -D warnings
passes (needless borrows in audit_module, dead code, unused imports,
fixtures lints)
Docs
docs/deployment.md: new "Deployment Parameter Validation" section with
the rule table, error codes, and network-id operator steps
docs/sdk-interface-spec.md: DeploymentError reference table, updated
initialize entries, documented set_network_id / get_network_id
docs/deployment-verification.md,
docs/interop/client-fallback-behavior.md: replaced stale "panics with
Already initialized" references with the typed errors
Privacy considerations
No private data is exposed: initialization events carry only public
configuration addresses, and the network identifier is an operator-chosen
public label. Covered by a dedicated test
(dp_initialized_event_exposes_only_public_config).
Testing
cargo fmt --all -- --check # clean
cargo clippy --workspace --all-targets -D warnings # clean
cargo test --workspace # 310 passed, 0 failed
(cli excluded locally: openssl-sys cannot build in this environment without
pkg-config/OpenSSL; it builds in CI.)