Skip to content

refactor(contract): use internal config::Config type instead of dto - #4278

Merged
gilcu3 merged 2 commits into
mainfrom
4055-updateconfig-stores-the-wire-dto-instead-of-the-internal-config
Aug 28, 2026
Merged

refactor(contract): use internal config::Config type instead of dto#4278
gilcu3 merged 2 commits into
mainfrom
4055-updateconfig-stores-the-wire-dto-instead-of-the-internal-config

Conversation

@gilcu3

@gilcu3 gilcu3 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Closes #4055

@gilcu3 gilcu3 linked an issue Aug 28, 2026 that may be closed by this pull request
3 tasks
@gilcu3
gilcu3 marked this pull request as ready for review August 28, 2026 08:17
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Pull request overview

Update::Config was storing the wire DTO (near_mpc_contract_interface::types::Config) even though Update is persisted in contract state via ProposedUpdates. That forced the propose-time validation added in #3564 to be a validate-and-discard (let _: crate::config::Config = config.clone().try_into()?;). This PR changes the variant to hold the internal crate::config::Config, so validation becomes a plain fallible conversion, and narrows Update / ProposedUpdates to pub(crate). Closes #4055.

Changes:

  • Update::Config now carries crate::config::Config instead of the interface DTO.
  • TryFrom<ProposeUpdateArgs> for Update reduces to Update::Config(config.try_into()?); the throwaway validation and its explanatory comment are removed.
  • Update and ProposedUpdates narrowed from pub to pub(crate).
  • Unit tests construct Update::Config via try_into().unwrap().

Reviewed changes

Per-file summary
File Description
crates/contract/src/update.rs Update::Config holds the internal Config; validate-and-discard replaced by the conversion itself; Update/ProposedUpdates made pub(crate); two tests updated to convert dummy_config before constructing the variant
crates/contract/src/api/update.rs test_proposed_updates_case_given_state builds Update::Config from the converted internal type (hash still computed over the DTO, so the equivalence stays pinned)

Verification notes

Checked the things that could have broken silently:

  • State layout — internal Config (crates/contract/src/config.rs:53) and DTO Config (crates/near-mpc-contract-interface/src/types/config.rs:82) have the same 19 u64 fields in the same order, and the Update discriminants are unchanged, so persisted ProposedUpdates entries deserialize identically. No migration needed.
  • UpdateHash::Configcrates/contract/src/dto_mapping.rs:434 hashes serde_json::to_vec(config); identical field names and order mean the externally visible hash is byte-identical. crates/contract/src/api/update.rs:303-309 still derives the expected hash from the DTO, so that equivalence stays asserted.
  • update_config self-callcrates/contract/src/update.rs:210 serializes the internal config into a call typed dtos::Config; JSON is identical, and tests/sandbox/upgrade_from_current_contract.rs::test_propose_update_config exercises the full propose -> vote -> apply -> read-back path.
  • Visibility narrowing — no out-of-crate uses of Update/ProposedUpdates; both MpcContract definitions (crates/contract/src/lib.rs:59, crates/contract/src/v3_14_0_state.rs:235) keep the field private, so no private_interfaces fallout. Neither type is reachable from the ABI, and the Config/ProposedUpdates entries in abi__abi_has_not_changed.snap are the DTOs.
  • Test fixturesdummy_config sets launcher_hash_unused_ttl_seconds to 14 days, which is >= the 7-day DEFAULT_EXPIRATION_DURATION_SECONDS, so the new try_into().unwrap() calls do not panic.

Findings

Non-blocking (nits, follow-ups, suggestions):

  • crates/contract/src/update.rs:210do_update now JSON-serializes the internal Config as the argument for update_config(config: dtos::Config). Before this PR the stored value was the DTO, so the self-call payload was type-correct by construction; now it rests on the two structs staying JSON-identical, with nothing at compile time enforcing it. If they ever diverge, the failure lands at apply time in a separate receipt — after do_update has already cleared every pending proposal, which is the exact hazard feat(contract): auto-expire unused launcher image hashes #3564 hardened against. Issue Update::Config stores the wire DTO instead of the internal Config #4055 anticipated converting explicitly here, and it is cheap to make type-checked:

    Update::Config(config) => {
        let new_config_gas_value = Gas::from_tgas(config.contract_upgrade_deposit_tera_gas);
        let config: dtos::Config = (&config).into();
        promise = promise.function_call(
            method_names::UPDATE_CONFIG,
            serde_json::to_vec(&(&config,)).unwrap(),
            // ...

    The same implicit coupling now governs the externally visible UpdateHash::Config at crates/contract/src/dto_mapping.rs:434; converting there too would make the "clients hash the DTO they submitted" contract explicit rather than coincidental. Both paths are covered by tests today, hence non-blocking.

  • crates/contract/src/update.rs:86 — dropping the feat(contract): auto-expire unused launcher image hashes #3564 comment leaves the ? looking like an incidental type conversion, when it is the only thing rejecting an unusable config before do_update clears the pending proposals. The type change makes the stored value validated by construction, but it does not make the propose-time rejection self-evident, and the fields of Config are pub(crate), so the internal type is still constructible unvalidated inside the crate. update_try_from__should_reject_invalid_config_at_propose_time (crates/contract/src/update.rs:307) documents and guards it, so this is a judgment call — a one-line note on impl TryFrom<ProposeUpdateArgs> for Update would keep the why next to the code without restating it.

✅ Approved

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR restructures internal code to use the internal Config type instead of DTO types. Since no bug is being fixed, the type prefix should probably be refactor: instead of fix:.

Suggested title: refactor(contract): use internal config::Config type instead of dto

@gilcu3 gilcu3 changed the title fix(contract): use internal config::Config type instead of dto refactor(contract): use internal config::Config type instead of dto Aug 28, 2026
@gilcu3
gilcu3 enabled auto-merge August 28, 2026 09:02
@gilcu3
gilcu3 added this pull request to the merge queue Aug 28, 2026
Merged via the queue into main with commit d19846f Aug 28, 2026
17 checks passed
@gilcu3
gilcu3 deleted the 4055-updateconfig-stores-the-wire-dto-instead-of-the-internal-config branch August 28, 2026 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update::Config stores the wire DTO instead of the internal Config

3 participants