fix(dips): recover a missing terms_version_hash from chain before cancel - #691
Open
ayushsingh82 wants to merge 1 commit into
Open
fix(dips): recover a missing terms_version_hash from chain before cancel#691ayushsingh82 wants to merge 1 commit into
ayushsingh82 wants to merge 1 commit into
Conversation
edgeandnode#638 asked for three protections around protocol-managed cancellation silently no-opping on a stale/wrong terms hash. Two of the three (confirming the cancel took effect via a post-cancel getAgreementDetails read, and surfacing a warning instead of staying silent) already landed in edgeandnode#643 after the issue was filed. The third — recovering a lost hash for pre-migration rows instead of leaving them permanently uncancelable — was still missing. cancel_agreement_on_chain now falls back to ChainClient::fetch_agreement_version_hash (getAgreementDetails(id, VERSION_CURRENT).versionHash) when the local terms_version_hash is absent, uses the recovered hash for the cancel, and best-effort persists it via a new AgreementRegistry::update_terms_version_hash so future cancels don't need to re-fetch. If the contract has no hash on record either, the call still fails with MissingTermsVersionHash as before. Also rewords the compute_terms_version_hash doc comment: in protocol-managed mode dipper signs nothing, so the value is only the cancel identifier the collector stored, not "the hash dipper signs over" — and a mismatch is now caught by cancel confirmation rather than failing silently, per the other edgeandnode#638 cleanup item. Closes edgeandnode#638
Author
|
@MoonBoi9001 opened this against #638 — would appreciate a look when you have a moment. |
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.
Closes #638.
Summary
#638 asked for three protections around protocol-managed cancellation silently no-opping on a stale/wrong terms hash:
getAgreementDetailsafter a mined cancel).terms_version_hashfor pre-migration rows instead of leaving them permanently uncancelable.Items 1 and 2 already landed in #643 (
CancelNotConfirmed+ every call site logging atracing::warn!/tracing::error!and retaining the agreement for retry) — that PR merged after this issue was filed but never explicitly closed it. Item 3 was still missing: aMissingTermsVersionHashagreement had no path back to being cancelable.Changes
ChainClient::fetch_agreement_version_hash— readsgetAgreementDetails(id, VERSION_CURRENT).versionHash, refactored to share the RPC call/decode with the existingagreement_still_active.AgreementRegistry::update_terms_version_hash— new PostgresUPDATE, guarded onterms_version_hash IS NULLso it can only backfill, never clobber a real stored value.cancel_agreement_on_chainnow resolves the version hash via a newresolve_version_hashhelper: use the local one if present and 32 bytes; otherwise fetch it from chain, use it for this cancel, and best-effort persist it (a persistence failure just logs a warning — the cancel still proceeds with the recovered hash). If the contract has no hash on record either, it still fails withMissingTermsVersionHashas before.compute_terms_version_hashdoc comment (one of the issue's minor cleanups): in protocol-managed mode dipper signs nothing, so the value is only the cancel identifier the collector stored — not "the hash dipper signs over" — and a mismatch is now caught by cancel confirmation rather than failing silently. (The other minor cleanup, giving test fakes a real state field instead of an "already cancelled" result the contract never produces, turned out to already be fixed —chain_listener.rs'sMockChainClient::cancel_via_manageralready documents and models the real no-op behavior.)cancel_agreement_on_chainand theChainClient/AgreementRegistrytraits gained one parameter/method respectively, which is why this touches 14 files — mostly one-method additions to 8 existing test mocks.Test plan
manager_cancel_recovers_missing_hash_from_chain_and_persists_it), and recovery finds nothing on-chain either (manager_cancel_missing_hash_with_nothing_on_chain_is_still_missing_hash_error)cargo test --workspace 'tests::' -- --skip 'tests::it_'— 421 passed, 0 failed (333 in dipper-service, including the 2 new ones)just check(cargo clippy -- -D warnings --force-warn deprecated --force-warn dead-code) — cleanjust fmt-check(cargo +nightly fmt --all -- --check) — cleancargo build(full workspace) — clean