feat: transfer ownership best-effort datastore lookup#2194
Open
chris-de-leon-cll wants to merge 7 commits into
Open
feat: transfer ownership best-effort datastore lookup#2194chris-de-leon-cll wants to merge 7 commits into
chris-de-leon-cll wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes ownership-transfer changesets to treat datastore contract-ref enrichment (FindAndFormatRef) as best-effort, allowing the ownership sequences to proceed when callers provide an address-only datastore.AddressRef that is not present in the datastore.
Changes:
- In
acceptOwnershipApply, normalize refs, attempt datastore enrichment, and fall back to the caller-provided ref on lookup failure. - In
transferOwnershipApply, apply the same best-effort enrichment behavior for contract refs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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.
Reduce datastore lookups in TransferOwnership / AcceptOwnership
Background
TransferOwnershipChangesetandAcceptOwnershipChangesetaccept a list ofContractRefentries per chain. Before executing the ownership sequence, each ref was passed throughFindAndFormatRef— a datastore lookup that resolves a partial ref into a fully-populatedAddressRef(withType,Version, andQualifierfilled in from stored metadata). This was a hard failure: if the ref was not present in the datastore, the changeset would error out even when the caller had already supplied the contract address directly.The only field the downstream ownership sequence actually needs from a ref is
Address.ContractTypeis used in a single log line to identify the contract being operated on;VersionandQualifierare not consumed at all.What this PR does
The
FindAndFormatRefcall is demoted from required to best-effort whenAddressis already set on the ref. The datastore is still consulted first — a hit produces a richer ref that improves log output. A miss emits a warning and the caller-supplied ref is passed through unchanged; the ownership transfer proceeds either way. WhenAddressis not set, the datastore lookup remains mandatory and returns a hard error on failure, preserving the original behavior for callers that rely on the DS to resolve partial refs.Design choices
DS as cache, not gatekeeper (when address is known). The datastore enrichment is purely additive here — it provides
Typefor a log message, nothing more. Making a successful log line a precondition for an on-chain ownership transfer is a stronger requirement than the operation actually needs. The revised behavior matches howResolveTokenPoolReftreats the datastore throughout the rest of the codebase: try it, fall back gracefully on a miss.Hard-fail preserved for address-less refs. If a caller provides a ref with no
Address, the DS lookup must succeed — silently proceeding with an empty address would passAddress=""into the ownership sequence and produce a confusing failure downstream. TheAddress != ""guard makes the two code paths explicit.Backwards compatible. No interface changes. Callers that already populate full refs (DS hit path) see identical behavior. Callers that pass type/version-only refs (DS must resolve path) see identical behavior. Only callers that provide an address but whose ref is absent from the DS see a change: hard error → warn and proceed.
Normalization is still required.
TryNormalizeAddressRefruns unconditionally before the DS attempt, so address formatting (e.g. EVM checksumming) is always applied regardless of whether the DS lookup succeeds.