Skip to content

Evm watcher unit tests#4776

Merged
mdulin2 merged 11 commits into
wormhole-foundation:mainfrom
mdulin2:evm-watcher-processNewBlock-tests
Jun 26, 2026
Merged

Evm watcher unit tests#4776
mdulin2 merged 11 commits into
wormhole-foundation:mainfrom
mdulin2:evm-watcher-processNewBlock-tests

Conversation

@mdulin2

@mdulin2 mdulin2 commented Apr 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a small rewrite that moves large amounts of code from goroutines into a separate function to make it more testable.

The main reason for this is to make the code more testable. This includes a large set of unit tests for the EVM watcher paths from this.

There are two main non-test changes:

  • Error handing on tx == nil receipt used to be deleted from the pending list. However, transient errors also return a nil receipt. So, I changed the logic to actually have transient errors stay in the watcher.
  • Added a receipt.txhash == pending.txhash check. This should probably never happen but with the amount of chains we support, we need to be sure.

@mdulin2 mdulin2 marked this pull request as draft April 28, 2026 21:57
@johnsaigle

johnsaigle commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@mdulin2 This should be good to rebase and continue working on now that your other PR was merged.

@mdulin2 mdulin2 force-pushed the evm-watcher-processNewBlock-tests branch from c4598c3 to ed3f4d2 Compare May 1, 2026 17:37
mdulin2
mdulin2 commented May 1, 2026
View reviewed changes
Comment thread node/pkg/watchers/evm/watcher.go
mdulin2
mdulin2 commented May 1, 2026
View reviewed changes
Comment thread node/pkg/watchers/evm/watcher.go Outdated
@mdulin2 mdulin2 marked this pull request as ready for review May 1, 2026 17:42
johnsaigle
johnsaigle reviewed May 1, 2026
View reviewed changes

@johnsaigle johnsaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the refactor looks good, just left a couple of suggestions.

Comment thread node/pkg/watchers/evm/watcher.go Outdated
Comment thread node/pkg/watchers/evm/watcher.go Outdated
@johnsaigle

johnsaigle commented May 1, 2026

Copy link
Copy Markdown
Contributor

Please also run make update-coverage-baseline and commit the results so that we can lock-in the improved coverage 🔒 ✅

pleasew8t
pleasew8t previously approved these changes May 6, 2026
View reviewed changes
@mdulin2 mdulin2 force-pushed the evm-watcher-processNewBlock-tests branch from 621ca71 to 4372703 Compare May 20, 2026 20:45
@mdulin2 mdulin2 requested a review from johnsaigle June 3, 2026 15:40
@johnsaigle johnsaigle force-pushed the evm-watcher-processNewBlock-tests branch from 4372703 to 8388f5f Compare June 4, 2026 12:42
johnsaigle
johnsaigle previously approved these changes Jun 4, 2026
View reviewed changes

@johnsaigle johnsaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous comments are addressed.

djb15
djb15 reviewed Jun 4, 2026
View reviewed changes

@djb15 djb15 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactor looks safe and happy to see more tests! Before I approve, what's the reasoning for removing some of the prior SECURITY comments? I like the explicitness and in prior conversations we agreed to flag critical checks in this way so they're less likely to be removed in future changes

@mdulin2

mdulin2 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

I'll add in the SECURITY comment back in. No problem.

@mdulin2 mdulin2 force-pushed the evm-watcher-processNewBlock-tests branch from 8388f5f to b9cf198 Compare June 5, 2026 20:45
@mdulin2

mdulin2 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

I messed up the history of the PR with something else I'm working on. I'll fix this in a little bit.

@mdulin2 mdulin2 force-pushed the evm-watcher-processNewBlock-tests branch 2 times, most recently from 18e1954 to a01a34b Compare June 5, 2026 23:36
@mdulin2

mdulin2 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Rebase shouldn't include the Aptos PR changes anymore. This should be rebased on main.

djb15
djb15 previously approved these changes Jun 8, 2026
View reviewed changes
johnsaigle
johnsaigle reviewed Jun 8, 2026
View reviewed changes
Comment thread node/pkg/watchers/evm/watcher.go Outdated
Comment thread node/pkg/watchers/evm/watcher.go
johnsaigle
johnsaigle previously approved these changes Jun 10, 2026
View reviewed changes
djb15
djb15 previously approved these changes Jun 10, 2026
View reviewed changes
mdulin2 added 9 commits June 11, 2026 07:32
@mdulin2 @johnsaigle
EVM watcher tests + testability
2cf0832 
* Seperated main code within goroutines for message processing into separate functions.
* Added tests for processBlock (non-instant path) and instant path on EVM.
@mdulin2 @johnsaigle
Add reobservation test cases on EVM
c4771e1
@mdulin2 @johnsaigle
EVM watcher test changes + watcher hardening
2d58076 
* Added new tests to deal with previous hardening changes
* Changed error handling on tx == nil within processNewBlock in order to allow for transient retries
* Added stored txhash is the same as the found txhash that was just queried.
@mdulin2 @johnsaigle
Fix lints and reinitialization bug on network stats
18702a5
@mdulin2 @johnsaigle
Fix gofmt error
c68ffa4
@mdulin2 @johnsaigle
Fixed channelcheck comment and renamed tx to txreceipt in watcher pro…
9ec13b3 
…cessing
@mdulin2 @johnsaigle
Add security comments back in
907344a
@mdulin2 @johnsaigle
Add security comments back in
ff7be85
@mdulin2 @johnsaigle
Better error handling on receipt == nil and logging bad status
960d194
@johnsaigle johnsaigle force-pushed the evm-watcher-processNewBlock-tests branch from 547e2e3 to 960d194 Compare June 11, 2026 11:32
panoel
panoel previously approved these changes Jun 24, 2026
View reviewed changes
@mdulin2 mdulin2 dismissed stale reviews from panoel, djb15, and johnsaigle via 4b1d9fc June 24, 2026 21:10
johnsaigle
johnsaigle approved these changes Jun 25, 2026
View reviewed changes

@johnsaigle johnsaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving. Minor changes since my last approval.

@mdulin2 mdulin2 merged commit 5406b6a into wormhole-foundation:main Jun 26, 2026
72 of 74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnsaigle johnsaigle johnsaigle approved these changes
@panoel panoel panoel approved these changes
@pleasew8t pleasew8t pleasew8t left review comments
@djb15 djb15 djb15 left review comments
@evan-gray evan-gray Awaiting requested review from evan-gray evan-gray is a code owner
@kcsongor kcsongor Awaiting requested review from kcsongor kcsongor is a code owner
@SEJeff SEJeff Awaiting requested review from SEJeff SEJeff is a code owner
@bemic bemic Awaiting requested review from bemic bemic is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mdulin2 @johnsaigle @pleasew8t @djb15 @panoel