fix(traits): derive real verifier selectors instead of placeholder bytes - #1237
Merged
nanaf6203-bit merged 2 commits intoSep 26, 2026
Merged
nanaf6203-bit merged 2 commits into
nanaf6203-bit merged 2 commits into
Conversation
`verification_selector` shipped `[0x01..=0x04, 0, 0, 0]` and documented itself as a stub to be replaced "once the underlying verification contract messages are finalised". They are finalised, and the placeholders had become load-bearing anyway: any tooling that trusted them dispatched to four non-existent messages, and a wrong selector fails silently rather than reverting, so the failure would have shown up as a verifier that never answers. Each kind now resolves to `ink::selector_bytes!` of the live message: Identity -> identity::IdentityRegistry::verify_identity Compliance -> compliance_registry::ComplianceRegistry::is_compliant Sanctions -> sanctions::SanctionsScreening::is_property_screened Oracle -> oracle::PropertyOracle::get_property_valuation Delegating to the macro rather than pasting four byte literals is the point. A literal is correct only until someone renames a message on the other side, and then it is wrong in a way that still compiles. The macro recomputes from the same ink version the targets use, so the two cannot drift. All four targets are top-level messages in their contract module rather than nested in a submodule, which is what makes the un-namespaced form of the macro the matching one. Adds `verification_message` so callers and error text can name the message they are calling instead of presenting an opaque selector, and documents that the four targets do not share a signature: two are keyed by AccountId, two by u64 property id, and the oracle returns a valuation struct rather than a bool. Real selectors make dispatch land on the right message; they do not make the arguments line up, and the docs now say so instead of implying the flow is uniform. `aggregate_verifications_appends_input_after_selector_bytes` asserted `&[0x01, 0, 0, 0]` directly, so it pinned the placeholder and had to be rewritten to compare against `verification_selector`. Closes MettaChain#1182 Closes MettaChain#1179 Closes MettaChain#1180 Closes MettaChain#1181
|
@Spaully 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
Replaces the placeholder verification selectors with selectors derived from the live verifier messages.
Closes #1182
Closes #1179
Closes #1180
Closes #1181
Why
verification_selectorreturned[0x01..=0x04, 0, 0, 0]and documented itself as a stub "to be replaced once the underlying verification contract messages are finalised". They are finalised, and in the meantime the placeholders became load-bearing: any tooling that trusted them dispatched to four non-existent messages.The failure mode is what makes this worth fixing rather than documenting. A wrong selector does not revert — it dispatches to a message that does not exist, or worse to a different message that does. So the symptom would have been a verifier that silently never answers, not an error.
What changed
contracts/traits/src/multicall.rs— each kind now resolves toink::selector_bytes!of a real message:Identityidentity::IdentityRegistry::verify_identityCompliancecompliance_registry::ComplianceRegistry::is_compliantSanctionssanctions::SanctionsScreening::is_property_screenedOracleoracle::PropertyOracle::get_property_valuationWhy the macro and not four byte literals
This is the actual design decision. A pasted literal is correct only until someone renames a message on the other side — and then it is wrong in a way that still compiles, still type-checks, and still dispatches. That is precisely the bug being fixed.
ink::selector_bytes!recomputes from the same ink version the target contracts use, so the two cannot drift. I verified all four targets are top-level messages in their contract module rather than nested in a submodule, which is what makes the un-namespaced form of the macro the matching one. (identity,compliance_registryandsanctionsare root-levellib.rscrates here, notsrc/lib.rs;oracleissrc/lib.rs.)Message-name choices
The issue did not say which message each kind should target, so these are judgement calls:
is_compliantoverrequire_compliancefor Compliance — it returnsboolrather than reverting, which is what a batched check wants. A reverting callee insidetry_aggregate_callswould just be recorded as a failure.is_property_screenedfor Sanctions — also aboolpredicate, so it matches the shape ofis_compliant. See the caveat below; this is the weakest of the four mappings.The four targets do not share a signature
Real selectors make the dispatch land on the right message. They do not make the arguments line up, and I did not want the docs implying otherwise:
verify_identityandis_compliantare keyed byAccountId.is_property_screenedandget_property_valuationare keyed byu64property id.get_property_valuationreturns a valuation struct, not abool.So
build_verification_call's single opaqueinput: &[u8]is not uniformly meaningful across kinds. A caller cannot pass one payload that is correct for all four. That is a pre-existing gap in the flow that this PR documents rather than papers over — closing it means per-kind typed arguments, which is a signature change tobuild_verification_calland out of scope here.This is also why the
Sanctionsmapping is the shakiest: the kind is documented as "verify the actor is not on any sanctions list", but the boolean message available is property-keyed. If actor-keyed sanctions is wanted, it needs a new message on the sanctions contract, not a different selector here.verification_messageAdded so callers and error text can name the message being called instead of presenting an opaque four-byte selector. The names and the
selector_bytes!literals are kept in step by a test.Tests
6 new tests, plus one existing test rewritten.
Rewritten:
aggregate_verifications_appends_input_after_selector_bytesasserted&[0x01, 0, 0, 0]directly, so it was pinning the placeholder. It now compares againstverification_selector(VerificationKind::Identity). This is the test that would have caught the original stub, had it been written that way.New:
each_kind_binds_to_its_live_verifier_selectoranddocumented_message_names_hash_to_their_selectors— bind each kind to the selector of the message it names, so editing one without the other is caught here rather than on-chain.no_selector_is_a_placeholder— none of the old[0x01..=0x04, 0, 0, 0]values can come back.no_selector_is_zero_padded— catches a stub introduced with different numbering, since a real selector is a hash prefix.real_selectors_are_still_pairwise_distinct— the stubs guaranteed distinctness by hand; real hash-derived values have to genuinely not collide, so the property is re-asserted against real bytes.every_kind_names_its_verifier_message.The pre-existing
selectors_are_distinct_across_kindsis retained unchanged and now exercises real values.Note: this and #1233 touch the same file
#1233 (Kandexa,
SelectorTooShort) also editscontracts/traits/src/multicall.rs. This branch is cut frommainand does not contain it. Both add an#[cfg(test)] mod testsblock at the end of the same file, so whichever merges second will need a reconcile — most likely the two test blocks concatenating.Integration changes
verification_selectorreturns different bytes than before. Any off-chain caller that hard-coded the placeholders must be updated — which is the point, but worth stating for the release note.VerificationKindis unchanged: no new variants, no discriminant movement.verification_message.CallRequest/CallResultlayout change, no new dependencies, noCargo.lockchange.build_verification_callandaggregate_verificationsare unchanged.Test plan
cargo test -p propchain-traits— 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.cargo clippy --workspace --all-targets -- -D warnings— not run, same reason.cargo build --workspace— not run, same reason.The 6 tests added here are expected to compile and pass, but they are unverified.
The one thing a reviewer should sanity-check on a real toolchain is that the four
selector_bytes!literals resolve to the same four bytes the target contracts compute — that is the entire premise of the PR, and it is the kind of cross-crate fact I could only establish by reading, not by building. If any target message turns out to be namespaced, that one selector would need the namespaced macro form.Env vars