test(dpp): pin V0 config parser consensus-frozen quirk#3514
Conversation
`DataContractConfigV0::get_contract_configuration_properties_v0` read both `requires_identity_encryption_bounded_key` and `requires_identity_decryption_bounded_key` from the same map key (`REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY`), so V0 configs parsed from a property map could never set the two fields to different values. V1's parser is correct; V0 had a copy-paste typo dating back to the initial contract bounds feature in #1358. The V0 dispatcher is currently dead (no active callers; parent is marked `TODO: Remove, it's not using`), so the fix cannot alter any already-ingested contract state or consensus hashes. Added regression tests covering distinct values, absence, and decryption-only.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChanges add unit tests for V0 data contract configuration properties parsing and document the intentional behavior where decryption-bounded-key values are sourced from the encryption constant in the V0 parser, ensuring historical behavior consistency. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reverts the functional change from the prior commit on this branch.
`DataContractConfigV0::get_contract_configuration_properties_v0` reads
the decryption bounded-key field from the ENCRYPTION property key. This
is not a safe-to-patch typo: its behavior is part of V0 protocol output
and cannot be changed without risking a chain fork on replay. V1's
parser is already correct and handles the DECRYPTION key distinctly.
- Restore the original V0 lookup.
- Add a `CONSENSUS-FROZEN BUG` comment at the site so the quirk is not
re-"fixed" by a future refactor.
- Replace the earlier regression tests with a lock-in test module that
asserts the observable V0 behavior:
* encryption property populates BOTH fields,
* decryption property is ignored entirely,
* encryption wins when both are set,
* neither set leaves both `None`.
QuantumExplorer
changed the title
Review GateCommit:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3514 +/- ##
============================================
+ Coverage 87.29% 87.30% +0.01%
============================================
Files 2476 2476
Lines 285752 285766 +14
============================================
+ Hits 249454 249501 +47
+ Misses 36298 36265 -33
🚀 New features to boost your workflow:
|
# Conflicts: # packages/rs-dpp/src/data_contract/config/mod.rs
|
Reviewed AI based PR |
2 participants
Issue being fixed or feature implemented
While investigating packages/rs-dpp/src/data_contract/config/v0/mod.rs:177-185, I noticed that
get_contract_configuration_properties_v0reads the decryption bounded-key field fromREQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY(not the matching DECRYPTION constant). V1 at packages/rs-dpp/src/data_contract/config/v1/mod.rs:140 reads from the correct key.It looks like a copy-paste typo from #1358 (2023-09), but it is part of V0 protocol behavior and must not be changed: altering the V0 parser's output would risk a fork on any node replaying historical V0 contracts, even if the function appears dead today.
So this PR does not fix the bug. Instead it locks the existing V0 behavior in place so nobody accidentally "fixes" it later.
What was done?
CONSENSUS-FROZEN BUGcomment directly at the offending line in V0, pointing at V1 as the correct reference.get_contract_configuration_properties_v0_consensus_locktest module asserting the current V0 behavior:None.No functional code change — V0 parsing behavior is unchanged. V1 is untouched.
Commit history
This branch has two commits on it. The first commit (
fix(dpp): ...) was an incorrect attempt to patch the typo before I realized the consensus implications. The second commit (test(dpp): ...) reverts that change and replaces the regression tests with lock-in tests. Both kept for transparency; happy to squash before merge.How Has This Been Tested?
Breaking Changes
None. Behavior of the V0 parser is preserved exactly.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Documentation