Add datastore write API for ccip - #23600
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ No conflicts with other open PRs targeting |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a4474056b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| finalCSOut.DataStore = ds | ||
| if finalCSOut.DataStore == nil { | ||
| finalCSOut.DataStore = singletons | ||
| } else if err := finalCSOut.DataStore.Merge(singletons.Seal()); err != nil { |
There was a problem hiding this comment.
When a sub-changeset records an address using a semantic qualifier, its legacy address-book entry is also passed to PopulateDataStore, which creates another ref for the same contract using the synthetic address-type qualifier. Merging all of those refs unconditionally leaves two keys for the same chain/type/version/address, so lookups that omit a qualifier become ambiguous and the returned datastore no longer preserves the intended write model. Filter address-book-derived refs whose contract is already represented; the same unconditional merge appears in both Solana E2E variants.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Last PR removes this completely. No need to address individually.
| for _, ref := range srcRefs { | ||
| if held, taken := heldBy[ref.Key().String()]; taken && !shared.AddressesEqual(ref.ChainSelector, held, ref.Address) { |
There was a problem hiding this comment.
When src stages a RemoteDelete, its memory store can retain the corresponding address record while tracking the key in DeletedRemoteKeys—the deletion test constructs exactly this shape. If an earlier sub-changeset has updated that key to a different address, this comparison rejects the later deletion as an address collision even though MemoryDataStore.Merge would delete the key and no source address would survive. Remove deletion-marked refs from collision checks (and environment publication) so valid ordered replacement/deletion sequences can be orchestrated.
Useful? React with 👍 / 👎.
| // An address the environment already records under the same type and version is not | ||
| // re-saved: AddressBookMap.Save rejects a repeated address, and a changeset that registers | ||
| // contracts the environment already knows about is doing so legitimately. An address | ||
| // recorded under a *different* type and version is a genuine disagreement and is rejected | ||
| // here, before anything is merged. | ||
| for _, chainSelector := range sortedKeys(srcAddrs) { |
There was a problem hiding this comment.
Is there a reason we can't re-use mergeAddressBookEntries here? The code seems almost identical
There was a problem hiding this comment.
mergeAddressBookEntries cannot be reused directly because it mutates an AddressBook, while this code is only planning and must not mutate env.ExistingAddresses. This loop also needs to collect new entries into p.envAddresses
| // planAddressBook stages the merged address book in a fresh book, so the destination is only | ||
| // replaced once the whole merge is known to be legal, and works out which entries the | ||
| // environment is still missing. |
There was a problem hiding this comment.
| // planAddressBook stages the merged address book in a fresh book, so the destination is only | |
| // replaced once the whole merge is known to be legal, and works out which entries the | |
| // environment is still missing. | |
| // planAddressBook stages the merged address book in a fresh book, so the destination is only | |
| // replaced once the whole merge is known to be legal, and works out which entries the | |
| // environment is still missing. | |
| // The merged book consists of addresses in dest, src, and env.ExistingAddresses |
| "failed to merge data store: addresses %s and %s both claim %s; give them distinct qualifiers", | ||
| earlier.Address, ref.Address, key, |
There was a problem hiding this comment.
I thought it was impossible to actually construct a datastore w/ two entries that have the exact same address, chain, and type and version w/out a differentiating qualifier.
There was a problem hiding this comment.
Yes but this check is for the same datastore key with different addresses
| // The environment is the state before this run, so a source ref taking over a key it holds | ||
| // is a redeploy superseding what was there, not a collision. It is recorded so the | ||
| // replacement is reported rather than silent. |
There was a problem hiding this comment.
Is it always the case that we want to overwrite?
There was a problem hiding this comment.
overwriting is not unconditional. But source precedence is intentional only after conflict validation. The merge uses Upsert because it is applying the valid later state.
8b12f73 to
5dffd2c
Compare
5dffd2c to
a25f320
Compare
4 participants
CCIP-13348
Introduces the CCIP-owned datastore write API and guarded changeset-output merge path required for the AddressBook retirement. This establishes one consistent write model before converting individual changesets. Datastore keys are now caller-defined and validated before writes occur.
Requires
This is the base PR of the stack.
Supports
Stack created with GitHub Stacks CLI • Give Feedback 💬