feat(commitment_core): harden commitment ID generation and uniqueness strategy#463
Open
daveades wants to merge 3 commits intoCommitlabs-Org:masterfrom
Conversation
- Change generate_commitment_id prefix from "c_" to "COMMIT_" for clarity and consistency with the protocol naming convention. - Add DuplicateCommitmentId (error 25) guard in create_commitment: if the counter-derived ID already exists in storage the call panics before any state is written, preventing silent overwrites caused by storage corruption or unexpected counter resets. - Expose commitment_id_exists(id) as a public read-only query so off-chain integrators can verify ID existence before calling create_commitment. - Add AuthorizedUpdaters variant to DataKey enum (was missing, causing require_authorized_updater / add_authorized_updater / remove_authorized_updater to fail to compile). - Add MAX_PAGE_SIZE constant (was missing, caused list functions to fail to compile). - Fix add_allocator bug: used undeclared `contract_address` instead of `allocator` parameter. - Remove four duplicate creation-fee / net-amount calculations in create_commitment; keep only the first (checked arithmetic). - Re-export emit_error_event, SafeMath, Pausable, RateLimiter, TimeUtils, Validation from shared_utils crate root so commitment_core can import them as flat symbols. - Fix pre-existing compilation errors in test modules: missing Ledger import in emergency_tests/benchmark_invariant_tests, std::vec::Vec usage in no_std context, wrong arg-count calls to get_owner_commitments (changed to list_commitments_by_owner), missing caller arg to update_value, missing TokenClient import in fee_tests, undefined expected_returned variable in fee_tests. - Update benchmark_invariant_tests ID-format assertions from "c_N" to "COMMIT_N". - Add five new tests covering uniqueness, format, exists-query, and monotonic counter behaviour. - Mark the two addressed KNOWN_LIMITATIONS items as fixed. closes Commitlabs-Org#204
|
@daveades 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! 🚀 |
The Cargo.lock regeneration (which fixed the duplicate version-system entry) revealed pre-existing build failures in several contracts that were masked behind the lock-file parse error on master. Changes: - commitment_interface: add missing imports (category, Error) to test module; add CommitmentMetadata/CommitmentNFT to types.rs; update get_owner_commitments expected signature to include offset/limit params - commitment_nft: add missing shared_utils imports (Pausable, EmergencyControl, SafeMath); remove unused rand dependency - price_oracle: add Vec to soroban_sdk imports; shorten function name get_price_for_high_value_operation (34 chars) to get_price_high_value_op (22 chars, Soroban max is 32); fix borrow-after-move by cloning Env; replace nonexistent SafeMath::calculate_percentage_change with inline calc - commitment_marketplace: fix listing.payment_token reference before listing is defined — use payment_token parameter directly - allocation_logic: remove Address::is_zero() call (method does not exist on soroban_sdk::Address; auth is already enforced by require_auth) - attestation_engine: remove extra false argument from two write_attestation calls (function takes 6 args, was being called with 7)
This reverts commit 6f8bedc.
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
1 participant
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.
Summary
generate_commitment_idnow producesCOMMIT_<counter>(wasc_<counter>). Unambiguous, consistent with the documented plan.create_commitmentchecks the generated ID does not already exist. NewDuplicateCommitmentId(error 25) prevents silent overwrite on counter/storage corruption.commitment_id_exists(id): New public read-only query for off-chain callers.commitment_core(AuthorizedUpdatersmissing,MAX_PAGE_SIZEundefined,contract_addresstypo, missingshared_utilsre-exports).fee_tests,emergency_tests,benchmark_invariant_tests.commitment_id_existslifecycle, nonexistent ID returns false, monotonic sequence.version-systementry (pre-existing, blocking all CI on master before this PR).CI failures are pre-existing and not caused by this PR
Both CI checks were already failing on master before this PR due to the Cargo.lock parse error. After the lock file is fixed, two different pre-existing failures become visible:
commitment_interfacetests have missing imports and structs absent fromtypes.rs. Pre-existing bugs unrelated to this PR.commitment_nft,price_oracle,attestation_engine,allocation_logic,commitment_marketplacehave compilation errors that were masked by the Cargo.lock parse failure. Pre-existing, unrelated to commitment ID generation.Security notes
commitment_id_existsis read-only, no auth required.Test results (commitment_core)
14 pre-existing failures remain (wrong assertion values in fee tests, allocate not updating TVL, event topic mismatches) — none related to this PR.
closes #204