fix(factory): validate init_params and version before deploying - #1236
Merged
nanaf6203-bit merged 1 commit intoSep 26, 2026
Conversation
`deploy_contract` passed `config.init_params` straight to `builder::build_contract`, which ignores the payload and returns `Ok` unconditionally, and it stored `version` with no check at all. So `Error::InvalidParameters` was unreachable dead code: it existed to tell callers their parameters were checked, and nothing checked them. Two things were wrong with that. An oversized `init_params` was copied into the transaction, re-encoded for the cross-contract call, and then written into the deployment record's footprint with nothing to stop it, and an unbounded `version` is unbounded permanent storage in `DeployedContract`, paid for by the factory. `deploy_contract` now rejects an empty or over-long `init_params` with `InvalidParameters`, and an empty or over-long `version` with a new `InvalidVersion` so a caller can tell which half of the request was rejected. Validation runs before the code-hash lookup, so a malformed call is reported on its own terms instead of surfacing as `CodeHashNotSet` and sending the caller hunting for a problem that does not exist. Rejecting *empty* params is the judgement call in this change. The factory stores only a `Hash` per type, not the constructor metadata, so it cannot type-check the payload against the expected ABI, and an empty argument list is only wrong if the target constructor takes arguments. It is still the right default: `build_contract` discards the payload, so a deployment registered with no constructor arguments is unverifiable by construction, and no `DeploymentTemplate` in `templates.rs` encodes to an empty payload, so this matches the intended calling convention. If some `ContractType` genuinely needs a zero-argument constructor, the follow-up is a per-type policy rather than relaxing the global rule -- noted in the PR. `MAX_INIT_PARAMS_LEN` (4 KiB) and `MAX_VERSION_LEN` (64) are defined here rather than in `propchain-traits` because this crate has no `propchain-traits` dependency, and both limits are readable on-chain via a new `deployment_limits()` so a client can pre-validate instead of discovering them from a failed transaction. This changes one existing test fixture: `escrow_config` supplied `init_params: Vec::new()`, which is exactly the input now rejected. Closes MettaChain#1175 Closes MettaChain#1176 Closes MettaChain#1177 Closes MettaChain#1178
|
@merlik787-droi 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.
Summary
Validates
init_paramsandversionindeploy_contract, makingError::InvalidParametersreachable for the first time.Closes #1175
Closes #1176
Closes #1177
Closes #1178
Why
deploy_contractpassedconfig.init_paramsstraight tobuilder::build_contract, which ignores the payload entirely and returnsOkunconditionally.versionwas stored with no check at all.Error::InvalidParameterswas therefore unreachable dead code — it existed to tell callers their parameters had been checked, and nothing checked them. That is worse than having no variant: it is a variant that actively misleads an integrator reading the error enum.Two concrete problems beyond the dead variant:
init_paramswas copied into the transaction, re-encoded for the cross-contract call, and written into the deployment record's footprint, with nothing rejecting it at any point.versionis unbounded permanent storage inDeployedContract, paid for by the factory and returned byget_deployment. One call with a 1 MB version string is a permanent storage subsidy.What changed
contracts/factory/src/lib.rsMAX_INIT_PARAMS_LEN = 4_096andMAX_VERSION_LEN = 64, both documented with the reasoning and the template sizes they sit well above.deploy_contractrejects:init_params→InvalidParameters(now actually constructed)version→InvalidVersion(new, appended last)validate_deployment_request— a private associated function holding the rules.deployment_limits()— a read-only message so a client can check a request before submitting it rather than discovering the limits from a failed transaction.The judgement call: rejecting empty
init_paramsThis is the part of the change worth reviewing carefully, because it is the one behaviour that is not unambiguously correct.
The factory stores only a
Hashper contract type, not its constructor metadata. It therefore cannot type-check the payload against the expected ABI, and an empty argument list is only actually wrong if the target constructor takes arguments. AContractTypewith a genuinely zero-argument constructor would now be undeployable.I rejected empty anyway, for three reasons:
build_contractdiscards the payload, so a deployment registered with no constructor arguments is unverifiable by construction. Nothing downstream can go back and check that the target accepted them.DeploymentTemplateintemplates.rsencodes to an empty payload — they all emit at least a token name and symbol. So requiring non-empty matches the intended calling convention rather than inventing a new one.If a
ContractTypedoes turn out to need a zero-argument constructor, the correct follow-up is a per-type policy (arequires_params(ContractType) -> boolconsulted by the validator), not relaxing the rule globally. I did not build that here because it is speculative — no such type exists today — and it would widen the diff considerably. Flagging it rather than silently guessing.Other decisions
CodeHashNotSetand sending the caller hunting for a code-hash problem that does not exist. Pinned bytest_validation_precedes_the_code_hash_check, withtest_well_formed_request_still_reports_the_missing_code_hashguarding the other direction so the existing guard still fires.InvalidVersionis separate fromInvalidParameters. The issue suggested reusingInvalidParametersfor both, but telling a caller their version string was empty when their params were fine is not useful. Appended last, so no existing discriminant moves.init_paramsis checked beforeversion, so a caller fixing one problem at a time gets a deterministic first error.propchain-traits. The issue mentions "shared constants", but this crate has nopropchain-traitsdependency, and adding one is aCargo.tomlchange plus a hand-editedCargo.lock— not something to do blind in a PR about parameter validation.Tests
13 new tests, plus one existing fixture updated.
Updated:
escrow_configsuppliedinit_params: Vec::new(), which is now rejected, so it supplies a small non-empty payload. This was the only existing test affected, and the fixture was itself an instance of the bug — it demonstrated the factory happily recording a deployment with no constructor arguments.New:
init_params→InvalidParameters; over-long →InvalidParameters; exactly at the limit → accepted and recorded;version→InvalidVersion; over-long →InvalidVersion; exactly at the limit → accepted and recorded verbatim;init_paramsis reported beforeversion;CodeHashNotSet;deployment_countstays 0, the deployer list stays empty, and no deployment record exists;Not addressed here
Two things in this issue's block are out of scope for this PR and remain open elsewhere:
build_contractstill returnsAccountId::from([0u8; 32]), so a successful deployment is still registered against an un-callable address. That is the separate zero-address issue, and it needs real instantiation plumbing. This PR does not paper over it and does not add a zero-address rejection, which would make every deployment fail against the current stub.deployer_contracts. Per-deployer growth is a storage/gas concern about the registry, not about validating a request.Integration changes
InvalidVersion, appended last.deployment_limits().Cargo.lockchange.set_code_hash,change_admin, the getters, and the recording logic are untouched.Test plan
cargo test -p propchain-factory— not run. No code validation was performed, by explicit instruction; this change is source-only and manually reviewed.cargo fmt --all -- --check— not run, same reason. The new helperconfig_withhas a long signature thatrustfmtwould wrap.cargo clippy --workspace --all-targets -- -D warnings— not run, same reason.cargo build --workspace— not run, same reason.The 13 tests added here are expected to compile and pass, but they are unverified. The pre-existing tests were read and their expectations checked against the new validation by hand;
test_deploy_without_code_hash_failsandtest_deployment_getters_reflect_recorded_deploymentswere the two that could have broken, and both are accounted for above.Env vars