Open
Conversation
📝 WalkthroughWalkthroughThis PR migrates imports from Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as Sender / Relayer
participant Transceiver as WormholeTransceiver
participant Core as ICoreBridge
participant NttMgr as NttManager
Sender->>Core: publish signed VAA (encoded bytes)
Core->>Transceiver: deliver encodedMessage (bytes calldata)
Transceiver->>Core: CoreBridgeLib.decodeAndVerifyVaaCd(encodedMessage)
Core-->>Transceiver: (emitterChainId, emitterAddress, payload)
Transceiver->>Transceiver: validate peer (getWormholePeer(emitterChainId) == emitterAddress)
Transceiver->>Transceiver: vaaDigest = VaaLib.calcVaaDoubleHashCd(encodedMessage)
Transceiver->>Transceiver: check/set VAA consumed (isVAAConsumed / _setVAAConsumed)
Transceiver->>NttMgr: call attestation/receive with decoded payload
NttMgr-->>Transceiver: acknowledge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
evm/src/libraries/TransceiverStructs.sol (1)
58-64:⚠️ Potential issue | 🟡 MinorKeep
nttManagerMessageDigestaligned with the wire-format length bound.This helper now hashes
uint16(m.payload.length)without preserving thetype(uint16).maxguard thatencodeNttManagerMessage()still enforces below. For oversized payloads, the digest helper will now succeed on a message that the encoder rejects, so the two APIs no longer agree on what constitutes a validNttManagerMessage.💡 Proposed fix
function nttManagerMessageDigest( uint16 sourceChainId, NttManagerMessage memory m ) public pure returns (bytes32) { + if (m.payload.length > type(uint16).max) { + revert PayloadTooLong(m.payload.length); + } return keccak256( abi.encodePacked(sourceChainId, m.id, m.sender, uint16(m.payload.length), m.payload) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm/src/libraries/TransceiverStructs.sol` around lines 58 - 64, The digest helper nttManagerMessageDigest is inconsistent with encodeNttManagerMessage because it hashes uint16(m.payload.length) without the encoder's type(uint16).max clamp; update nttManagerMessageDigest to compute the payload length exactly as the encoder does (i.e., clamp the length to type(uint16).max before casting) so both functions use the same wire-format length bound; locate nttManagerMessageDigest and mirror the encoder's min/clip logic when producing the uint16 length value for the keccak256 input.
🧹 Nitpick comments (2)
evm/test/IntegrationStandalone.t.sol (1)
670-680: Centralize_getSignedMessages.This helper is now copied into multiple integration tests and overlaps with
IntegrationHelpers._getWormholeMessage. Pulling it into one shared utility will make future harness updates much easier to keep in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm/test/IntegrationStandalone.t.sol` around lines 670 - 680, Duplicate helper _getSignedMessages should be removed and callers should use the centralized IntegrationHelpers._getWormholeMessage instead; update IntegrationStandalone.t.sol to delete the local _getSignedMessages function, import or inherit IntegrationHelpers where needed, and replace all calls to _getSignedMessages(logs, emitterChainId) with IntegrationHelpers._getWormholeMessage(logs, emitterChainId) (or the instance method name as defined in IntegrationHelpers) ensuring types/visibility match and any WormholeOverride/VaaLib dependencies are resolved via the shared helper.evm/test/IntegrationCustomConsistency.t.sol (1)
304-315: Extract_getSignedMessagesinto a shared test helper.The same helper now exists here and in
evm/test/Upgrades.t.sol, so any future change to the WormholeOverride/VAA signing path has to be kept in sync in multiple suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm/test/IntegrationCustomConsistency.t.sol` around lines 304 - 315, Duplicate test helper _getSignedMessages (which calls WormholeOverride.fetchPublishedMessages, sets PublishedMessage.envelope.emitterChainId, calls WormholeOverride.sign and VaaLib.encode) should be extracted to a single shared test helper; create a shared library/contract (e.g., TestWormholeHelpers.getSignedMessages) that accepts Vm.Log[] and uint16 emitterChainId, implements the same logic, make it internal/view or public/view as needed for tests, then replace the duplicate implementations in both IntegrationCustomConsistency.t.sol and Upgrades.t.sol with calls to the shared helper so future changes to WormholeOverride/VAA signing are centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@evm/echidna/FuzzNttManager.sol`:
- Line 17: The import in FuzzNttManager.sol now references
"wormhole-sdk/Utils.sol" but the wormhole-solidity-sdk submodule is not
initialized so remapped path (evm/remappings.txt mapping wormhole-sdk/ ->
lib/wormhole-solidity-sdk/src/) will fail; either ensure the submodule is
initialized in CI/setup by running git submodule update --init --recursive
before compilation, or revert the import to the previously working path (e.g.,
the original wormhole-solidity-sdk import) so builds work without submodule
changes; update CI/build scripts or developer setup docs accordingly and adjust
FuzzNttManager.sol import to match whichever approach you choose.
In `@evm/lib/wormhole-solidity-sdk`:
- Line 1: The submodule at evm/lib/wormhole-solidity-sdk is pinned to the wrong
commit (d80e0f6cd16f281f3f9df435e98e823b4212b2ad) which doesn't match the
intended v1.1.0 tag (33d4895fb629aabf12e0f60740367bd03e702bd1); update the
submodule pointer so the gitlink points to the v1.1.0 tag commit (33d4895f...),
or if a different commit is intended, replace the pinned SHA with that correct
commit and update .gitmodules/commit reference and the submodule state in the
repo so the submodule resolves to the declared commit. Ensure the commit SHA in
the PR matches the upstream tag or include a clear justification in the PR
description if you intentionally pin to a non-tagged commit.
In `@evm/test/IntegrationAdditionalTransfer.t.sol`:
- Around line 226-228: The test currently uses a bare vm.expectRevert() before
calling wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) which will pass
for any revert; update this to assert the specific duplicate-delivery revert by
swapping vm.expectRevert() for vm.expectRevert(<expectedRevertBytesOrString>)
where <expectedRevertBytesOrString> is the exact revert message or encoded error
selector thrown by receiveMessage on duplicate deliveries (use the contract's
DuplicateDelivery error name or revert string encoded with
abi.encodeWithSignature/abi.encodePacked as appropriate) so the test only
succeeds if the second receiveMessage call fails for the intended
duplicate-delivery reason.
In `@evm/test/IntegrationStandalone.t.sol`:
- Around line 222-224: Replace the broad vm.expectRevert() with a revert
expectation that matches the replay error selector, e.g.
vm.expectRevert(abi.encodeWithSelector(<REPLAY_ERROR>.selector)); for the call
to wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) (and do the same for
the second occurrence around lines 344-346). Use the actual replay error
type/identifier declared in the transceiver contract (replace <REPLAY_ERROR>
with that error name) so the test asserts the revert is specifically from the
replay-path.
In `@evm/test/IntegrationWithoutRateLimiting.t.sol`:
- Around line 203-205: Replace the generic vm.expectRevert() checks with
assertions that check for the specific replay-protection revert so the test
guarantees the failure is due to duplicate delivery; for the calls to
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) (and the same pattern at
the other location), use vm.expectRevert(...) with the contract's replay error
selector or encoded revert reason (e.g.,
vm.expectRevert(abi.encodeWithSelector(ReplayAlreadyProcessed.selector)) or
vm.expectRevert(bytes("AlreadyProcessed"))), then call
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) so the test explicitly
validates the replay-protection error.
In `@evm/test/Upgrades.t.sol`:
- Around line 513-514: The test currently uses a generic vm.expectRevert()
before calling wormholeTransceiverChain2.receiveMessage(encodedVMs[0]), which
can pass for any failure; replace the generic expectRevert with a specific
revert assertion that matches the contract's replay-protection error (either the
exact revert string or the error selector/ABI-encoded bytes for the replay
error) so the test proves the second delivery hit replay protection; update the
call that precedes wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) to
use vm.expectRevert(...) with the replay-specific payload (e.g., the contract
error selector or revert message) referencing the replay protection error
defined in the transceiver contract.
---
Outside diff comments:
In `@evm/src/libraries/TransceiverStructs.sol`:
- Around line 58-64: The digest helper nttManagerMessageDigest is inconsistent
with encodeNttManagerMessage because it hashes uint16(m.payload.length) without
the encoder's type(uint16).max clamp; update nttManagerMessageDigest to compute
the payload length exactly as the encoder does (i.e., clamp the length to
type(uint16).max before casting) so both functions use the same wire-format
length bound; locate nttManagerMessageDigest and mirror the encoder's min/clip
logic when producing the uint16 length value for the keccak256 input.
---
Nitpick comments:
In `@evm/test/IntegrationCustomConsistency.t.sol`:
- Around line 304-315: Duplicate test helper _getSignedMessages (which calls
WormholeOverride.fetchPublishedMessages, sets
PublishedMessage.envelope.emitterChainId, calls WormholeOverride.sign and
VaaLib.encode) should be extracted to a single shared test helper; create a
shared library/contract (e.g., TestWormholeHelpers.getSignedMessages) that
accepts Vm.Log[] and uint16 emitterChainId, implements the same logic, make it
internal/view or public/view as needed for tests, then replace the duplicate
implementations in both IntegrationCustomConsistency.t.sol and Upgrades.t.sol
with calls to the shared helper so future changes to WormholeOverride/VAA
signing are centralized.
In `@evm/test/IntegrationStandalone.t.sol`:
- Around line 670-680: Duplicate helper _getSignedMessages should be removed and
callers should use the centralized IntegrationHelpers._getWormholeMessage
instead; update IntegrationStandalone.t.sol to delete the local
_getSignedMessages function, import or inherit IntegrationHelpers where needed,
and replace all calls to _getSignedMessages(logs, emitterChainId) with
IntegrationHelpers._getWormholeMessage(logs, emitterChainId) (or the instance
method name as defined in IntegrationHelpers) ensuring types/visibility match
and any WormholeOverride/VaaLib dependencies are resolved via the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86b96402-bfb2-47df-a267-a7951ddd1f26
⛔ Files ignored due to path filters (1)
evm/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
evm/echidna/FuzzNttManager.solevm/foundry.tomlevm/lib/wormhole-solidity-sdkevm/remappings.txtevm/script/DeployWormholeNtt.s.solevm/script/helpers/DeployWormholeNttBase.solevm/script/helpers/ParseNttConfig.solevm/src/NttManager/ManagerBase.solevm/src/NttManager/NttManager.solevm/src/Transceiver/Transceiver.solevm/src/Transceiver/WormholeTransceiver/WormholeTransceiver.solevm/src/Transceiver/WormholeTransceiver/WormholeTransceiverState.solevm/src/interfaces/IWormholeTransceiver.solevm/src/libraries/ConfigMakers.solevm/src/libraries/TransceiverHelpers.solevm/src/libraries/TransceiverStructs.solevm/src/wormhole/Governance.solevm/test/DeploymentScripts.t.solevm/test/IntegrationAdditionalTransfer.t.solevm/test/IntegrationCustomConsistency.t.solevm/test/IntegrationStandalone.t.solevm/test/IntegrationWithoutRateLimiting.t.solevm/test/NttManager.t.solevm/test/NttManagerNoRateLimiting.t.solevm/test/RateLimit.t.solevm/test/Upgrades.t.solevm/test/libraries/IntegrationHelpers.solevm/test/libraries/NttManagerHelpers.solevm/test/libraries/TransceiverHelpers.solevm/test/mocks/MockNttManagerAdditionalPayload.solevm/test/wormhole/Governance.t.solevm/ts/src/bindings.tsevm/ts/src/ethers-contracts/2_1_0/NttManager.tsevm/ts/src/ethers-contracts/2_1_0/WormholeTransceiver.tsevm/ts/src/ethers-contracts/2_1_0/common.tsevm/ts/src/ethers-contracts/2_1_0/factories/NttManager__factory.tsevm/ts/src/ethers-contracts/2_1_0/factories/WormholeTransceiver__factory.tsevm/ts/src/ethers-contracts/2_1_0/factories/index.tsevm/ts/src/ethers-contracts/2_1_0/index.tsevm/ts/src/ethers-contracts/index.ts
| import "solidity-bytes-utils/BytesLib.sol"; | ||
| import "../test/mocks/DummyTransceiver.sol"; | ||
| import "wormhole-solidity-sdk/Utils.sol"; | ||
| import "wormhole-sdk/Utils.sol"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Utils.sol exists in the SDK submodule
fd -t f "Utils.sol" lib/wormhole-solidity-sdk/src/ 2>/dev/null
# Check what symbols are exported from Utils.sol
if [ -f "lib/wormhole-solidity-sdk/src/Utils.sol" ]; then
echo "=== Utils.sol contents ==="
head -50 lib/wormhole-solidity-sdk/src/Utils.sol
fiRepository: wormhole-foundation/native-token-transfers
Length of output: 68
🏁 Script executed:
#!/bin/bash
# Check lib directory structure
ls -la lib/ 2>/dev/null || echo "lib directory not found or empty"
# Check remappings.txt for relevant entries
echo "=== Checking remappings.txt ==="
cat remappings.txt 2>/dev/null || echo "remappings.txt not found"
# Search for Utils.sol anywhere in the repo
echo "=== Searching for Utils.sol ==="
fd -t f "Utils.sol" . 2>/dev/null | head -20Repository: wormhole-foundation/native-token-transfers
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Find and show FuzzNttManager.sol
echo "=== FuzzNttManager.sol location and content ==="
find . -name "FuzzNttManager.sol" -type f -exec head -25 {} \;
# Check for remapping configurations in config files
echo -e "\n=== Checking config files for remappings ==="
ls -la | grep -E "foundry|hardhat|truffle|forge"
if [ -f "foundry.toml" ]; then
echo "=== foundry.toml ==="
cat foundry.toml
fi
if [ -f "hardhat.config.ts" ] || [ -f "hardhat.config.js" ]; then
echo "=== hardhat config ==="
cat hardhat.config.* 2>/dev/null | head -50
fiRepository: wormhole-foundation/native-token-transfers
Length of output: 1240
🏁 Script executed:
#!/bin/bash
# Check for any config files that might contain remappings
echo "=== All config files in repo ==="
find . -maxdepth 2 -name "*.toml" -o -name "*.json" | grep -E "(forge|foundry|hardhat|package|tsconfig)" | head -20
# Check evm subdirectory specifically
echo -e "\n=== evm directory config files ==="
ls -la evm/ | grep -E "toml|json"
# Check package.json if it exists
if [ -f "package.json" ]; then
echo -e "\n=== package.json (first 80 lines) ==="
head -80 package.json
fi
if [ -f "evm/package.json" ]; then
echo -e "\n=== evm/package.json ==="
cat evm/package.json
fi
# Check for forge.toml
if [ -f "evm/forge.toml" ]; then
echo -e "\n=== evm/forge.toml ==="
cat evm/forge.toml
fiRepository: wormhole-foundation/native-token-transfers
Length of output: 2426
🏁 Script executed:
#!/bin/bash
echo "=== evm/foundry.toml ==="
cat evm/foundry.tomlRepository: wormhole-foundation/native-token-transfers
Length of output: 1948
🏁 Script executed:
#!/bin/bash
# Check evm package.json
if [ -f "evm/package.json" ]; then
echo "=== evm/package.json ==="
cat evm/package.json
else
echo "evm/package.json not found"
fi
# Check what's in evm/lib
echo -e "\n=== evm/lib directory ==="
ls -la evm/lib/ 2>/dev/null || echo "evm/lib directory not found"
# Search for any references to wormhole-sdk in the codebase
echo -e "\n=== References to wormhole-sdk ==="
grep -r "wormhole-sdk" evm/ --include="*.sol" --include="*.json" --include="*.toml" 2>/dev/null | head -20Repository: wormhole-foundation/native-token-transfers
Length of output: 2602
🏁 Script executed:
#!/bin/bash
# Check for wormhole-sdk as a symlink or directory
echo "=== Checking for wormhole-sdk ==="
ls -la evm/lib/ | grep wormhole
# Check if wormhole-sdk is referenced elsewhere
echo -e "\n=== Check git status for lib changes ==="
cd evm && git status lib/ 2>/dev/null | head -30
# Check what Utils.sol exports in the actual library
echo -e "\n=== Contents of wormhole-solidity-sdk/Utils.sol ==="
head -30 evm/lib/wormhole-solidity-sdk/Utils.sol 2>/dev/null || echo "File not found"
# Check if the imports are using the correct path in other files
echo -e "\n=== Sample working import from another file ==="
grep -A2 "import.*Utils" evm/src/NttManager/ManagerBase.sol | head -10Repository: wormhole-foundation/native-token-transfers
Length of output: 500
🏁 Script executed:
#!/bin/bash
# List contents of wormhole-solidity-sdk
echo "=== Contents of evm/lib/wormhole-solidity-sdk/ ==="
find evm/lib/wormhole-solidity-sdk -type f | head -20
# Check if it's actually a symlink
echo -e "\n=== Check symlinks in evm/lib ==="
ls -la evm/lib/
# Look for wormhole-sdk anywhere
echo -e "\n=== Search for wormhole-sdk anywhere ==="
find evm -name "wormhole-sdk" -o -name "wormhole-sdk*" 2>/dev/null
# Check actual location of Utils from test
echo -e "\n=== Utils.sol in test ==="
ls -la evm/test/libraries/ 2>/dev/null | grep UtilsRepository: wormhole-foundation/native-token-transfers
Length of output: 682
🏁 Script executed:
#!/bin/bash
# Verify wormhole-solidity-sdk is truly empty
echo "=== Checking if wormhole-solidity-sdk is a git submodule ==="
cd evm && git config --file .gitmodules --get-regexp path | grep wormhole
cd -
# Check the git history to see what changed
echo -e "\n=== Recent git log for FuzzNttManager.sol ==="
cd evm && git log --oneline -n 10 echidna/FuzzNttManager.sol 2>/dev/null || echo "No git history"
cd -
# Check if there's a remapping or import alias that might help
echo -e "\n=== Check for any remapping attempts in Solidity files ==="
grep -r "remapping\|import.*as" evm/src evm/echidna --include="*.sol" 2>/dev/null | head -5
# Verify: does wormhole-sdk exist as an npm package that would be installed?
echo -e "\n=== Check root package.json for wormhole-sdk ==="
grep -i "wormhole-sdk" package.json 2>/dev/null || echo "Not found in root package.json"Repository: wormhole-foundation/native-token-transfers
Length of output: 879
🏁 Script executed:
#!/bin/bash
# Check .gitmodules
echo "=== .gitmodules content ==="
cat .gitmodules 2>/dev/null || cat evm/.gitmodules 2>/dev/null || echo "No .gitmodules found"
# Check git submodule status
echo -e "\n=== Git submodule status ==="
git config --file .gitmodules --get-regexp . 2>/dev/null || echo "No submodules configured"
# Look at the actual commit that changed the imports
echo -e "\n=== Show the commit that changed FuzzNttManager.sol ==="
cd evm && git show --stat 3fa3e22 2>/dev/null | head -50
cd -Repository: wormhole-foundation/native-token-transfers
Length of output: 4278
🏁 Script executed:
#!/bin/bash
# Check the full diff of the commit to understand the remapping changes
echo "=== Full diff of the upgrade commit (first 200 lines) ==="
cd evm && git show 3fa3e22 -- foundry.toml 2>/dev/null | head -100
cd -
# Also check if remappings were added in this commit
echo -e "\n=== Check if remappings.txt was added ==="
cd evm && git show 3fa3e22 -- remappings.txt 2>/dev/null || echo "No remappings.txt in this commit"
cd -
# Check the actual current state - look at full diff for FuzzNttManager.sol
echo -e "\n=== Diff for FuzzNttManager.sol import change ==="
cd evm && git show 3fa3e22 -- echidna/FuzzNttManager.sol 2>/dev/null | grep -A5 -B5 "wormhole"
cd -Repository: wormhole-foundation/native-token-transfers
Length of output: 5327
🏁 Script executed:
#!/bin/bash
# Verify remappings.txt actually exists in current state
echo "=== Check if evm/remappings.txt exists ==="
ls -la evm/remappings.txt 2>/dev/null || echo "File does not exist in current state"
# Check git status to see if it's tracked
echo -e "\n=== Git status of remappings.txt ==="
cd evm && git ls-files remappings.txt 2>/dev/null && echo "File is tracked" || echo "File not tracked"
cd -
# Try to view the file directly
echo -e "\n=== Content of evm/remappings.txt ==="
cat evm/remappings.txt 2>/dev/null || echo "Cannot read file"
# Check git for current tracked files in evm root
echo -e "\n=== Tracked files in evm root ==="
cd evm && git ls-files | grep "^[^/]*$" | head -20
cd -Repository: wormhole-foundation/native-token-transfers
Length of output: 558
🏁 Script executed:
#!/bin/bash
# Verify Utils.sol exists at the remapped location
echo "=== Check if Utils.sol exists at remapped location ==="
ls -la evm/lib/wormhole-solidity-sdk/src/Utils.sol 2>/dev/null || echo "File not found"
# List what's in the src directory
echo -e "\n=== Contents of evm/lib/wormhole-solidity-sdk/src/ ==="
ls -la evm/lib/wormhole-solidity-sdk/src/ 2>/dev/null | head -20
# Verify the remapping resolves correctly
echo -e "\n=== Verify remapping resolution ==="
# Check if the path referenced in remappings exists
stat evm/lib/wormhole-solidity-sdk/src/ 2>/dev/null && echo "Remapped directory exists" || echo "Remapped directory not found"Repository: wormhole-foundation/native-token-transfers
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Check git submodule status
echo "=== Git submodule status ==="
cd evm && git submodule status 2>/dev/null
cd -
# Check if submodule initialization is needed
echo -e "\n=== Submodule update status ==="
cd evm && git config --file .gitmodules --get submodule.lib/wormhole-solidity-sdk.path
cd -
# Try to understand what's in the submodule directory
echo -e "\n=== Detailed check of wormhole-solidity-sdk ==="
find evm/lib/wormhole-solidity-sdk -type f 2>/dev/null | wc -l
echo "Number of files in wormhole-solidity-sdk:"
find evm/lib/wormhole-solidity-sdk -type d 2>/dev/null | wc -l
echo "Number of directories in wormhole-solidity-sdk (empty dirs count as 0 files)"Repository: wormhole-foundation/native-token-transfers
Length of output: 617
Critical: Submodule not initialized—remappings will fail to resolve.
The import path change to wormhole-sdk/Utils.sol depends on remappings configured in evm/remappings.txt, which maps wormhole-sdk/ to lib/wormhole-solidity-sdk/src/. However, the wormhole-solidity-sdk git submodule is not initialized in the current state (indicated by the - prefix in submodule status), meaning the src/ directory does not exist. This will cause compilation failures when Foundry/Forge attempts to resolve the import.
The remappings configuration itself is correct, but this PR requires either:
- Ensuring
git submodule update --init --recursiveruns before compilation (CI/build scripts, or developer setup instructions) - Or, if submodules are intentionally avoided, updating imports to use the old
wormhole-solidity-sdk/path which is already configured
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm/echidna/FuzzNttManager.sol` at line 17, The import in FuzzNttManager.sol
now references "wormhole-sdk/Utils.sol" but the wormhole-solidity-sdk submodule
is not initialized so remapped path (evm/remappings.txt mapping wormhole-sdk/ ->
lib/wormhole-solidity-sdk/src/) will fail; either ensure the submodule is
initialized in CI/setup by running git submodule update --init --recursive
before compilation, or revert the import to the previously working path (e.g.,
the original wormhole-solidity-sdk import) so builds work without submodule
changes; update CI/build scripts or developer setup docs accordingly and adjust
FuzzNttManager.sol import to match whichever approach you choose.
| @@ -1 +1 @@ | |||
| Subproject commit b9e129e65d34827d92fceeed8c87d3ecdfc801d0 | |||
| Subproject commit d80e0f6cd16f281f3f9df435e98e823b4212b2ad | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PINNED_SHA="d80e0f6cd16f281f3f9df435e98e823b4212b2ad"
echo "== gitlink in current tree =="
git ls-tree HEAD evm/lib/wormhole-solidity-sdk
echo
echo "== submodule entries from .gitmodules =="
git config -f .gitmodules --get-regexp '^submodule\..*\.(path|url)$'
SUBMODULE_URL="$(git config -f .gitmodules --get-regexp '^submodule\..*\.url$' | awk 'NR==1 {print $2}')"
echo
echo "== refs on upstream remote containing the pinned SHA =="
git ls-remote --tags --heads "$SUBMODULE_URL" | rg "$PINNED_SHA|refs/tags|refs/heads"Repository: wormhole-foundation/native-token-transfers
Length of output: 4420
Pinned SHA does not match the v1.1.0 release tag.
The submodule is pinned to commit d80e0f6cd16f281f3f9df435e98e823b4212b2ad, which does not correspond to the v1.1.0 tag (33d4895fb629aabf12e0f60740367bd03e702bd1) mentioned in the PR objective. The pinned SHA also does not appear in any upstream tag or branch on the Wormhole Solidity SDK repository. Please either update the pinned commit to match the intended v1.1.0 release tag or clarify which commit should be used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm/lib/wormhole-solidity-sdk` at line 1, The submodule at
evm/lib/wormhole-solidity-sdk is pinned to the wrong commit
(d80e0f6cd16f281f3f9df435e98e823b4212b2ad) which doesn't match the intended
v1.1.0 tag (33d4895fb629aabf12e0f60740367bd03e702bd1); update the submodule
pointer so the gitlink points to the v1.1.0 tag commit (33d4895f...), or if a
different commit is intended, replace the pinned SHA with that correct commit
and update .gitmodules/commit reference and the submodule state in the repo so
the submodule resolves to the declared commit. Ensure the commit SHA in the PR
matches the upstream tag or include a clear justification in the PR description
if you intentionally pin to a non-tagged commit.
Member
Author
There was a problem hiding this comment.
The diff from that commit is only npm publishing scripts. Not relevant.
There was a problem hiding this comment.
@dvgui, understood — if the only delta between the pinned commit (d80e0f6cd1) and the v1.1.0 tag is npm publishing scripts, then the Solidity/EVM content is effectively identical to v1.1.0 and the pin is fine. Thanks for the clarification!
✏️ Learnings added
Learnt from: dvgui
Repo: wormhole-foundation/native-token-transfers PR: 840
File: evm/lib/wormhole-solidity-sdk:1-1
Timestamp: 2026-03-12T20:23:23.293Z
Learning: In the wormhole-foundation/native-token-transfers repo, the evm/lib/wormhole-solidity-sdk submodule is intentionally pinned to a commit slightly ahead of the v1.1.0 tag (d80e0f6cd16f281f3f9df435e98e823b4212b2ad vs tag 33d4895fb629aabf12e0f60740367bd03e702bd1). The difference between those two commits is only npm publishing scripts, which are not relevant to EVM/Solidity usage. The Solidity content is effectively equivalent to v1.1.0.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
226
to
228
| // Can't resubmit the same message twice | ||
| (IWormhole.VM memory wormholeVM,,) = wormhole.parseAndVerifyVM(encodedVMs[0]); | ||
| vm.expectRevert( | ||
| abi.encodeWithSelector( | ||
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); | ||
| wormholeTransceiverChain2.receiveMessage(encodedVMs[0]); |
There was a problem hiding this comment.
Keep this replay check explicit.
A bare vm.expectRevert() will still pass if the second receive fails for the wrong reason, which weakens the duplicate-delivery assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm/test/IntegrationAdditionalTransfer.t.sol` around lines 226 - 228, The
test currently uses a bare vm.expectRevert() before calling
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) which will pass for any
revert; update this to assert the specific duplicate-delivery revert by swapping
vm.expectRevert() for vm.expectRevert(<expectedRevertBytesOrString>) where
<expectedRevertBytesOrString> is the exact revert message or encoded error
selector thrown by receiveMessage on duplicate deliveries (use the contract's
DuplicateDelivery error name or revert string encoded with
abi.encodeWithSignature/abi.encodePacked as appropriate) so the test only
succeeds if the second receiveMessage call fails for the intended
duplicate-delivery reason.
Comment on lines
222
to
224
| // Can't resubmit the same message twice | ||
| (IWormhole.VM memory wormholeVM,,) = wormhole.parseAndVerifyVM(encodedVMs[0]); | ||
| vm.expectRevert( | ||
| abi.encodeWithSelector( | ||
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); | ||
| wormholeTransceiverChain2.receiveMessage(encodedVMs[0]); |
There was a problem hiding this comment.
Keep these replay checks explicit.
vm.expectRevert() will also pass on unrelated failures, so these assertions no longer prove the second delivery is rejected by the replay path. Please match at least the selector here.
Also applies to: 344-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm/test/IntegrationStandalone.t.sol` around lines 222 - 224, Replace the
broad vm.expectRevert() with a revert expectation that matches the replay error
selector, e.g. vm.expectRevert(abi.encodeWithSelector(<REPLAY_ERROR>.selector));
for the call to wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) (and do
the same for the second occurrence around lines 344-346). Use the actual replay
error type/identifier declared in the transceiver contract (replace
<REPLAY_ERROR> with that error name) so the test asserts the revert is
specifically from the replay-path.
Comment on lines
203
to
205
| // Can't resubmit the same message twice | ||
| (IWormhole.VM memory wormholeVM,,) = wormhole.parseAndVerifyVM(encodedVMs[0]); | ||
| vm.expectRevert( | ||
| abi.encodeWithSelector( | ||
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); | ||
| wormholeTransceiverChain2.receiveMessage(encodedVMs[0]); |
There was a problem hiding this comment.
Keep these replay checks explicit.
vm.expectRevert() will pass on any revert, so these assertions no longer guarantee the second delivery is being rejected by replay protection specifically.
Also applies to: 377-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm/test/IntegrationWithoutRateLimiting.t.sol` around lines 203 - 205,
Replace the generic vm.expectRevert() checks with assertions that check for the
specific replay-protection revert so the test guarantees the failure is due to
duplicate delivery; for the calls to
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) (and the same pattern at
the other location), use vm.expectRevert(...) with the contract's replay error
selector or encoded revert reason (e.g.,
vm.expectRevert(abi.encodeWithSelector(ReplayAlreadyProcessed.selector)) or
vm.expectRevert(bytes("AlreadyProcessed"))), then call
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) so the test explicitly
validates the replay-protection error.
Comment on lines
+513
to
514
| vm.expectRevert(); | ||
| wormholeTransceiverChain2.receiveMessage(encodedVMs[0]); |
There was a problem hiding this comment.
Keep the replay assertion specific.
vm.expectRevert() now passes on any failure path, including bad VAA verification or fork mismatches, so this test no longer proves that the second delivery hit replay protection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm/test/Upgrades.t.sol` around lines 513 - 514, The test currently uses a
generic vm.expectRevert() before calling
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]), which can pass for any
failure; replace the generic expectRevert with a specific revert assertion that
matches the contract's replay-protection error (either the exact revert string
or the error selector/ABI-encoded bytes for the replay error) so the test proves
the second delivery hit replay protection; update the call that precedes
wormholeTransceiverChain2.receiveMessage(encodedVMs[0]) to use
vm.expectRevert(...) with the replay-specific payload (e.g., the contract error
selector or revert message) referencing the replay protection error defined in
the transceiver contract.
Member
Author
Findings 1-2 (FuzzNttManager import + submodule commit): Not real issues. wormhole-sdk/ is correctly remapped, and d80e0f6 IS the v1.1.0 tag. Findings 3-6 (generic vm.expectRevert()): vm.expectRevert(bytes4) fails for parameterized errors like TransferAlreadyCompleted(bytes32) in this forge-std version. Bare vm.expectRevert() is the correct approach. nttManagerMessageDigest length clamp: Not a real issue. The type(uint16).max check in encodeNttManagerMessage is a revert guard — if payload exceeds 65535 bytes, the encoder reverts. It never encodes a clamped value. So any valid message that passes through Nitpicks (duplicate _getSignedMessages helpers): These are refactoring suggestions, not bugs. The duplication exists because different test contracts inherit different bases and the helper is a few lines. Not worth the None of these findings require code changes. |
…ions Migrate from wormhole-solidity-sdk v0.1.0 to v1.1.0. Replace parseAndVerifyVM with client-side CoreBridgeLib.decodeAndVerifyVaaCd, switch receiveMessage to calldata, use VaaLib.calcVaaDoubleHashCd for replay protection, inline nttManagerMessageDigest, add constant-time countSetBits popcount, cache transceiver index in attestation, and replace ConfigMakers with SDK's CustomConsistencyLib. receiveMessage gas: avg -21.6%, median -18.1%, max -12.0%. Deployment size: -14.1%. Bump NttManager and WormholeTransceiver to v2.1.0 with TS SDK bindings.
quoteDeliveryPrice passed enabledTransceivers.length to parseTransceiverInstructions, causing index out of bounds after removing a transceiver. Use registered count instead, matching the correct pattern in _transferEntryPoint.
There was a problem hiding this comment.
Pull request overview
Upgrades the EVM contracts and tooling to Wormhole Solidity SDK v1.1.0, enabling solc 0.8.24 + via_ir, and adopts the SDK’s newer address and VAA verification utilities to reduce gas and deployment size while keeping ABI-versioned TS bindings in sync.
Changes:
- Upgrade build + remappings (SDK submodule rev,
solc0.8.24,via_ir = true,wormhole-sdk/imports). - Refactor Wormhole message verification to
CoreBridgeLib.decodeAndVerifyVaaCd+ replay digest viaVaaLib.calcVaaDoubleHashCd, plus calldata optimizations. - Migrate tests/scripts and TS ethers bindings to ABI
2.1.0and new WormholeOverride/VaaLib signing flow.
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| evm/ts/src/ethers-contracts/index.ts | Exposes new _2_1_0 ABI bindings. |
| evm/ts/src/ethers-contracts/2_1_0/index.ts | Adds autogenerated exports for ABI 2.1.0. |
| evm/ts/src/ethers-contracts/2_1_0/factories/index.ts | Adds factory exports for ABI 2.1.0. |
| evm/ts/src/ethers-contracts/2_1_0/factories/WormholeTransceiver__factory.ts | Adds ABI 2.1.0 WormholeTransceiver factory + bytecode linking. |
| evm/ts/src/ethers-contracts/2_1_0/common.ts | Adds typechain common typings for ABI 2.1.0. |
| evm/ts/src/ethers-contracts/2_1_0/WormholeTransceiver.ts | Adds ABI 2.1.0 WormholeTransceiver type definitions. |
| evm/ts/src/bindings.ts | Registers ABI version 2.1.0 as newest supported. |
| evm/test/wormhole/Governance.t.sol | Migrates to ICoreBridge + WormholeOverride/VaaLib-based VAA creation. |
| evm/test/mocks/MockNttManagerAdditionalPayload.sol | Switches to toUniversalAddress helper. |
| evm/test/libraries/TransceiverHelpers.sol | Updates helpers to universal addresses. |
| evm/test/libraries/NttManagerHelpers.sol | Updates peer setup to universal addresses. |
| evm/test/libraries/IntegrationHelpers.sol | Migrates to WormholeOverride/VaaLib and universal addresses. |
| evm/test/Upgrades.t.sol | Migrates to WormholeOverride/VaaLib and universal addresses; adjusts replay tests. |
| evm/test/RateLimit.t.sol | Migrates to WormholeOverride/VaaLib, BytesParsing API updates, universal addresses. |
| evm/test/NttManagerNoRateLimiting.t.sol | Migrates to WormholeOverride/VaaLib; adds cached chainid workaround for via_ir. |
| evm/test/NttManager.t.sol | Migrates to WormholeOverride/VaaLib; adds cached chainid workaround for via_ir. |
| evm/test/IntegrationWithoutRateLimiting.t.sol | Migrates to WormholeOverride/VaaLib and universal addresses; adjusts replay tests. |
| evm/test/IntegrationStandalone.t.sol | Migrates to WormholeOverride/VaaLib and universal addresses; adjusts replay tests. |
| evm/test/IntegrationCustomConsistency.t.sol | Switches to SDK CustomConsistency + WormholeOverride/VaaLib. |
| evm/test/IntegrationAdditionalTransfer.t.sol | Migrates to WormholeOverride/VaaLib and universal addresses; adjusts replay tests. |
| evm/test/DeploymentScripts.t.sol | Updates deployment script tests for SDK CustomConsistency + WormholeOverride. |
| evm/src/wormhole/Governance.sol | Replaces parseAndVerifyVM with SDK decode/verify + digest replay protection and updated BytesParsing API. |
| evm/src/libraries/TransceiverStructs.sol | Updates BytesParsing API; inlines message digest hashing. |
| evm/src/libraries/TransceiverHelpers.sol | Replaces popcount loop with constant-time popcount. |
| evm/src/libraries/ConfigMakers.sol | Updates BytesParsing API usage and import path. |
| evm/src/interfaces/IWormholeTransceiver.sol | Changes receiveMessage to bytes calldata for gas savings. |
| evm/src/Transceiver/WormholeTransceiver/WormholeTransceiverState.sol | Switches to ICoreBridge + SDK CustomConsistency helper + universal addresses. |
| evm/src/Transceiver/WormholeTransceiver/WormholeTransceiver.sol | Implements calldata receive + SDK VAA verification and replay digest changes; bumps version string. |
| evm/src/Transceiver/Transceiver.sol | Migrates to wormhole-sdk/Utils.sol and universal address comparisons. |
| evm/src/NttManager/NttManager.sol | Bumps version string and migrates to universal address helpers. |
| evm/src/NttManager/ManagerBase.sol | Caches transceiver index for attestation; adjusts instruction parsing length source. |
| evm/script/helpers/ParseNttConfig.sol | Removes local universal address helpers; imports from SDK. |
| evm/script/helpers/DeployWormholeNttBase.sol | Switches to ICoreBridge for messageFee() call. |
| evm/script/DeployWormholeNtt.s.sol | Switches to ICoreBridge for chainId/decimals sanity checks. |
| evm/remappings.txt | Adds wormhole-sdk/ remapping alongside legacy path. |
| evm/foundry.toml | Bumps solc_version to 0.8.24 and enables via_ir. |
| evm/foundry.lock | Updates wormhole SDK submodule revision. |
| evm/echidna/FuzzNttManager.sol | Migrates Utils import path to wormhole-sdk/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
vm.expectRevert() here is too broad for the replay-protection check; it will succeed for any revert. Please assert the specific IWormholeTransceiver.TransferAlreadyCompleted error with the expected VAA digest (computed from encodedVMs[0] using VaaLib.calcVaaDoubleHash*).
Suggested change
| vm.expectRevert(); | |
| bytes32 vaaDigest = VaaLib.calcVaaDoubleHash(encodedVMs[0]); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, | |
| vaaDigest | |
| ) | |
| ); |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
This replay-protection assertion uses vm.expectRevert() without checking the revert data, so it won’t catch regressions where receiveMessage reverts for an unexpected reason. Prefer asserting TransferAlreadyCompleted(expectedDigest) with the digest derived from encodedVMs[0] via VaaLib.calcVaaDoubleHash*.
Suggested change
| vm.expectRevert(); | |
| bytes32 expectedDigest = VaaLib.calcVaaDoubleHash(encodedVMs[0]); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, | |
| expectedDigest | |
| ) | |
| ); |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
vm.expectRevert() is overly permissive for validating replay protection; it will pass even if receiveMessage reverts for a different reason. Consider computing the VAA digest from encodedVMs[0] (via VaaLib.calcVaaDoubleHash*) and asserting the parameterized TransferAlreadyCompleted(digest) error.
Suggested change
| vm.expectRevert(); | |
| bytes32 digest = VaaLib.calcVaaDoubleHashFromEncoded(encodedVMs[0]); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| WormholeTransceiver.TransferAlreadyCompleted.selector, | |
| digest | |
| ) | |
| ); |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
This replay-submission check uses vm.expectRevert() without asserting the expected error selector/args, which weakens the test. Please assert IWormholeTransceiver.TransferAlreadyCompleted with the expected digest computed from encodedVMs[0] (e.g., via VaaLib.calcVaaDoubleHash*).
Suggested change
| vm.expectRevert(); | |
| bytes32 digest = VaaLib.calcVaaDoubleHash(encodedVMs[0]); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, | |
| digest | |
| ) | |
| ); |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
For the "can't resubmit" replay-protection case, vm.expectRevert() is too broad and can mask unintended revert causes. Prefer asserting the exact TransferAlreadyCompleted error with the expected digest (computed from encodedVMs[0] via VaaLib.calcVaaDoubleHash*).
Suggested change
| vm.expectRevert(); | |
| vm.expectRevert( | |
| abi.encodeWithSelector( | |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, | |
| VaaLib.calcVaaDoubleHash(encodedVMs[0]) | |
| ) | |
| ); |
Comment on lines
+62
to
+64
| return keccak256( | ||
| abi.encodePacked(sourceChainId, m.id, m.sender, uint16(m.payload.length), m.payload) | ||
| ); |
There was a problem hiding this comment.
nttManagerMessageDigest now casts m.payload.length to uint16 without enforcing the <= type(uint16).max invariant. Previously this digest path went through encodeNttManagerMessage(m), which reverts with PayloadTooLong when the payload is oversized. To avoid silent truncation (and potential digest collisions), add the same length check (or reuse encodeNttManagerMessage).
Suggested change
| return keccak256( | |
| abi.encodePacked(sourceChainId, m.id, m.sender, uint16(m.payload.length), m.payload) | |
| ); | |
| bytes memory encoded = encodeNttManagerMessage(m); | |
| return keccak256(abi.encodePacked(sourceChainId, encoded)); |
| IWormholeTransceiver.TransferAlreadyCompleted.selector, wormholeVM.hash | ||
| ) | ||
| ); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
vm.expectRevert() without specifying the expected error data makes this replay-protection assertion pass for any revert reason. Since receiveMessage now uses the VAA double-hash digest, compute the digest from encodedVMs[0] (via the appropriate VaaLib.calcVaaDoubleHash* helper) and assert TransferAlreadyCompleted(digest) explicitly.
Suggested change
| vm.expectRevert(); | |
| bytes32 digest = VaaLib.calcVaaDoubleHash(encodedVMs[0]); | |
| vm.expectRevert( | |
| abi.encodeWithSelector(WormholeTransceiver.TransferAlreadyCompleted.selector, digest) | |
| ); |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
evm/test/libraries/IntegrationHelpers.sol (1)
174-187: Rename_getWormholeMessageto match the array return.The helper now always returns multiple encoded VAAs, so the singular name makes the call sites read like a one-message path. Renaming it to
_getWormholeMessageswould make the helper’s contract clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm/test/libraries/IntegrationHelpers.sol` around lines 174 - 187, Rename the helper function _getWormholeMessage to the plural form _getWormholeMessages and update all references/call sites accordingly (including any tests or other helpers) so the name matches the bytes[] return; specifically change the function declaration for _getWormholeMessage and every invocation to _getWormholeMessages, keeping the implementation (PublishedMessage[], encodedVaas, WormholeOverride.fetchPublishedMessages, WormholeOverride.sign, VaaLib.encode) unchanged.evm/test/Upgrades.t.sol (1)
195-195: ExercisethisNttManagerafter the upgrade.
basicFunctionality()runs against the sharednttManagerChain1/2fixture, so this path currently only proves thatthisNttManager.upgrade(...)does not revert. A small post-upgrade assertion or local transfer flow onthisNttManagerwould make this test cover the special no-rate-limit upgrade path it sets up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm/test/Upgrades.t.sol` at line 195, After calling basicFunctionality() and performing thisNttManager.upgrade(...), actually exercise the upgraded instance: invoke a simple post-upgrade flow against thisNttManager (for example mint or transfer a token, or call a local transfer/send method and assert balances/ownership) and assert it succeeds; alternatively assert a relevant state change such as the peer rate limit set by thisNttManager.setPeer(chainId2, toUniversalAddress(address(0x1)), 9, type(uint64).max) is present/readable. Ensure the new assertions follow the upgrade call and reference thisNttManager, basicFunctionality(), upgrade(...), and setPeer(...) so the test verifies the no-rate-limit upgrade path rather than only checking for non-revert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@evm/test/libraries/IntegrationHelpers.sol`:
- Around line 174-187: Rename the helper function _getWormholeMessage to the
plural form _getWormholeMessages and update all references/call sites
accordingly (including any tests or other helpers) so the name matches the
bytes[] return; specifically change the function declaration for
_getWormholeMessage and every invocation to _getWormholeMessages, keeping the
implementation (PublishedMessage[], encodedVaas,
WormholeOverride.fetchPublishedMessages, WormholeOverride.sign, VaaLib.encode)
unchanged.
In `@evm/test/Upgrades.t.sol`:
- Line 195: After calling basicFunctionality() and performing
thisNttManager.upgrade(...), actually exercise the upgraded instance: invoke a
simple post-upgrade flow against thisNttManager (for example mint or transfer a
token, or call a local transfer/send method and assert balances/ownership) and
assert it succeeds; alternatively assert a relevant state change such as the
peer rate limit set by thisNttManager.setPeer(chainId2,
toUniversalAddress(address(0x1)), 9, type(uint64).max) is present/readable.
Ensure the new assertions follow the upgrade call and reference thisNttManager,
basicFunctionality(), upgrade(...), and setPeer(...) so the test verifies the
no-rate-limit upgrade path rather than only checking for non-revert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed45e5d9-0b1d-46b8-85d7-07ff5df0f744
⛔ Files ignored due to path filters (1)
evm/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
evm/echidna/FuzzNttManager.solevm/foundry.tomlevm/lib/wormhole-solidity-sdkevm/remappings.txtevm/script/DeployWormholeNtt.s.solevm/script/helpers/DeployWormholeNttBase.solevm/script/helpers/ParseNttConfig.solevm/src/NttManager/ManagerBase.solevm/src/NttManager/NttManager.solevm/src/Transceiver/Transceiver.solevm/src/Transceiver/WormholeTransceiver/WormholeTransceiver.solevm/src/Transceiver/WormholeTransceiver/WormholeTransceiverState.solevm/src/interfaces/IWormholeTransceiver.solevm/src/libraries/ConfigMakers.solevm/src/libraries/TransceiverHelpers.solevm/src/libraries/TransceiverStructs.solevm/src/wormhole/Governance.solevm/test/DeploymentScripts.t.solevm/test/IntegrationAdditionalTransfer.t.solevm/test/IntegrationCustomConsistency.t.solevm/test/IntegrationStandalone.t.solevm/test/IntegrationWithoutRateLimiting.t.solevm/test/NttManager.t.solevm/test/NttManagerNoRateLimiting.t.solevm/test/RateLimit.t.solevm/test/Upgrades.t.solevm/test/libraries/IntegrationHelpers.solevm/test/libraries/NttManagerHelpers.solevm/test/libraries/TransceiverHelpers.solevm/test/mocks/MockNttManagerAdditionalPayload.solevm/test/wormhole/Governance.t.solevm/ts/src/bindings.tsevm/ts/src/ethers-contracts/2_1_0/NttManager.tsevm/ts/src/ethers-contracts/2_1_0/WormholeTransceiver.tsevm/ts/src/ethers-contracts/2_1_0/common.tsevm/ts/src/ethers-contracts/2_1_0/factories/NttManager__factory.tsevm/ts/src/ethers-contracts/2_1_0/factories/WormholeTransceiver__factory.tsevm/ts/src/ethers-contracts/2_1_0/factories/index.tsevm/ts/src/ethers-contracts/2_1_0/index.tsevm/ts/src/ethers-contracts/index.ts
✅ Files skipped from review due to trivial changes (9)
- evm/echidna/FuzzNttManager.sol
- evm/foundry.toml
- evm/ts/src/ethers-contracts/index.ts
- evm/lib/wormhole-solidity-sdk
- evm/ts/src/ethers-contracts/2_1_0/index.ts
- evm/ts/src/ethers-contracts/2_1_0/factories/index.ts
- evm/remappings.txt
- evm/ts/src/ethers-contracts/2_1_0/factories/NttManager__factory.ts
- evm/ts/src/ethers-contracts/2_1_0/factories/WormholeTransceiver__factory.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- evm/test/mocks/MockNttManagerAdditionalPayload.sol
- evm/src/libraries/ConfigMakers.sol
- evm/src/interfaces/IWormholeTransceiver.sol
- evm/test/libraries/NttManagerHelpers.sol
- evm/src/Transceiver/Transceiver.sol
- evm/src/libraries/TransceiverHelpers.sol
- evm/ts/src/bindings.ts
- evm/test/RateLimit.t.sol
- evm/test/IntegrationAdditionalTransfer.t.sol
- evm/test/libraries/TransceiverHelpers.sol
- evm/src/NttManager/NttManager.sol
- evm/src/libraries/TransceiverStructs.sol
- evm/ts/src/ethers-contracts/2_1_0/WormholeTransceiver.ts
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.
Gas optimizations
Gas benchmarks —
WormholeTransceiver.receiveMessageDeployment size: 12,757 → 10,959 bytes (-14.1%)
Infrastructure
d80e0f6cd1)remappings.txtcreated forwormhole-sdk/pathssolcbumped from 0.8.19 to 0.8.24,via_irchanged fromfalsetotrue(required by SDK'sCoreBridgeLib)Build config
foundry.toml— solc 0.8.24, via_ir = truefoundry.lock— updated SDK commitremappings.txt— new filelib/wormhole-solidity-sdk— submodule updated to v1.1.0Source contracts (8 files)
IWormhole→ICoreBridge,toWormholeFormat→toUniversalAddress,fromWormholeFormat→fromUniversalAddressbytes memorynow haveMemsuffix (e.g.asBytes32Unchecked→asBytes32MemUnchecked)checkLengthsignature changed from(bytes memory, uint)to(uint, uint)— now called asBytesParsing.checkLength(encoded.length, offset)wormhole-solidity-sdk/towormhole-sdk/DeployWormholeNttBase.sol,DeployWormholeNtt.s.sol,ParseNttConfig.sol)Contract Changes
wormhole.parseAndVerifyVM()withCoreBridgeLib.decodeAndVerifyVaaCd()(client-side VAA verification — avoids external call overhead, more efficient ECDSA recovery loop, no ABI decode of VM struct). Uses the calldata variant to avoid copying the entire VAA into memory.receiveMessagecalldata switch: changedbytes memory→bytes calldataon bothIWormholeTransceiverandWormholeTransceiver, avoiding the implicit calldata-to-memory copy of the full encoded VAAVaaLib.calcVaaDoubleHashCd: replaced manualabi.encodePacked+ double-keccak with the SDK's optimized calldata hash utility, which hashes the VAA body directly from calldata usingkeccak256SliceUnchecked+keccak256Word(scratch space, no memory allocation). Produces the same backward-compatible double-keccak digest as the old CoreBridgeparseAndVerifyVM.nttManagerMessageDigestinlined: eliminated the intermediateencodeNttManagerMessageallocation by hashing struct fields directly viaabi.encodePacked(sourceChainId, m.id, m.sender, uint16(m.payload.length), m.payload)— produces identical digestcountSetBitsconstant-time popcount: replaced O(n) Kernighan loop with parallel bit counting (~6 ops, constant gas regardless of set bit count)_recordTransceiverAttestation: readstransceiverInfos[msg.sender].indexonce and reuses it for attestation check, bitmap set, and event emission — eliminates redundant warm SLOADsisVAAConsumedguard.SequenceReplayProtectionLibwas evaluated but not adopted — adding it alongside the hash guard (needed for upgrade safety) would increase gas costs rather than reduce them.decodeAndVerifyVaaCd+VaaLib.calcVaaDoubleHashCd+ calldata optimization; createdICoreBridgeGovernanceinterface extendingICoreBridgewith governance-specific methodsConfigMakerswith SDK'sCustomConsistencyLib(same encoding, more gas-efficient bit shifts vsabi.encodePacked). Replaced NTT'sICustomConsistencyLevelimport with SDK's identical interface.Test migration (11 test files + 1 fuzz file)
WormholeSimulator→WormholeOverridelibrary (completely different API)fetchPublishedMessages()+sign()+VaaLib.encode()pattern replaces oldfetchSignedMessageFromLogs()_getSignedMessages()(matching oldfetchSignedMessageFromLogs(log, chainId)behavior)vm.expectRevert()forTransferAlreadyCompleted(parameterized error)VaaBodystruct is now nested:{ VaaEnvelope envelope, bytes payload }instead of flat fieldsvia_irtest fixtest_transfersOnForkedChainsinNttManager.t.solandNttManagerNoRateLimiting.t.sol. Root cause: known Foundry/Solidityvia_iroptimizer bug where the Yul optimizer treatsCHAINIDas a pure/movable expression and may re-evaluate it aftervm.chainId()has changed the value. Fix: storeblock.chainidin a state variable duringsetUp()(storage reads are not subject to CSE/rematerialization).Summary by CodeRabbit
New Features
Improvements
Tests