node: Add watcher interface and refactor observation checks#4761
Draft
johnsaigle wants to merge 4 commits into
Draft
node: Add watcher interface and refactor observation checks#4761johnsaigle wants to merge 4 commits into
johnsaigle wants to merge 4 commits into
Conversation
Collaborator
|
I would strongly prefer having this split into per watcher PRs that we can review independently. We can keep this open as a draft to reference against and for easier comparison, but I think it's too meaty to garner prompt reviews. I would suggest maybe starting with the Solana watcher given the recent attention |
Contributor
Author
|
@djb15 I think I'll start with Aptos or Sui given that there are several other PRs open right now that modify the EVM and SVM watchers. if the idea is to split this into smaller chunks so we can move faster, I think that's the best way. |
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.
This PR is getting split into smaller PRs to help with code review.
Summary
Validate(...)methods and shared helpers, so fundamental request-shape and chain checks now happen in one consistent place instead of being scattered across reobservation handlers.PublishMessage(...)/PublishReobservation(...), folding older ad hoc patterns likeverifyAndPublishinto the new structure without changing the underlying message-processing control flow.Watcher Interface
node/pkg/watchers/watcher.go.ChainID()Validate(...)PublishMessage(...)PublishReobservation(...)Building and validating message publications has been left out of the design (at least for now) because the watchers all have very different ways of converting the on-chain data into MessagePublications such that I don't feel that it makes sense to generalize it into the interface.
However, the documentation in the interface notes that the watchers should also implement these methods in a consistent way.
Reviewer Notes
panic(...)cases on invalid observation requests were removed in favor of validated rejection and structured logging.Validate(...)PublishMessage(...)PublishReobservation(...)(which always callsPublishMessage()verifyAndPublishpattern in EVM/Sui is now subsumed into this architecture: verification remains, but publication flows through the common publish methods instead of a hybrid helper. (See node: Integrate Transfer Verifier into the Ethereum watcher #4233, node: Integrate Transfer Verifier into Sui Watcher #4324)If reviewers would be more comfortable with a smaller rollout, this change could be split into follow-up PRs per watcher or per watcher family. I kept it together because the repeated pattern is easier to validate side-by-side, and the targeted watcher test suite should help demonstrate that the refactor is safe.