V3 Internal Review #2733
Merged
V3 Internal Review #2733
Conversation
This file contains 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
Simplify mailbox and make interfaces payable Update version constant Keep message ID events
Use mailbox callback to authenticate messages in hooks where necessary (#2609) Co-authored-by: Kunal Arora <[email protected]>
- IGP as a standalone hook, implementing postDispatch to call payForGas directly - Setting a DEFAULT_GAS_USAGE if metadata not specified and message.senderAddress() as refund address if not specified. - None Fixes hyperlane-xyz/issues#511 Yes, same interface as the previous IGP but for Mailbox V3 Unit Tests --------- Co-authored-by: Yorke Rhodes <[email protected]>
- Updated the OP Stack tests for the Mailbox V3 transient storage version - Allowing OPStackHook to send msg.value at the time of message delivery (uses bit masking) - Added LibBit library, will be useful for all the auth hooks which can send `msg.value` - None - Fixes breaking OP Stack tests for hyperlane-xyz/issues#513 - Also fixes #2410 No Unit tests --------- Co-authored-by: Yorke Rhodes <[email protected]> Remove e2e and add yarn build to CI Adding protocol fees (#2640) - Adding protocol fee as a hook - None V3 Yes Fuzz tests --------- Co-authored-by: Yorke Rhodes <[email protected]>
- fixes GasRouter expectRevert message - added TestMerkleRootHook for proof() and fixes MerkleRootMultisig test - fixes ERC5164 tests post v3 changes - added contract name to mailbox and abstractHook error messages - removed unnecessary tests for "message too large" - downgrade slither in actions to 0.3.0 because of their recent "missing inheritance" bug on main - V3 Yes Unit tests
- `quoteDispatch` added to `IPostDispatchHook` interface and the
relevant hooks:
- StaticProtocolFee
- OPStackHook
- InterchainGasPaymaster
- MerkleTreeHook
- PausableHook (no tests)
- DomainRoutingHook (no tests)
- ConfigFallbackDomainRoutingHook
- ConfigurableDomainRoutingHook (no tests)
- `expectEmit` -> `expectCall`
- Quote in V3
Yes
Uint tests
### Description - AggregationHook for v3 ### Drive-by changes None ### Related issues v3 hyperlane-xyz/issues#514 ### Backward compatibility Yes ### Testing Always be fuzzing
- V3 compatible ERC20, ERC721 and all their extensions - Painstakingly migrating hardhat tests to foundry tests for all variants - fixes hyperlane-xyz/issues#577 Yes whole lotta unit tests and poor man's version of integration tests --------- Signed-off-by: -f <[email protected]> Co-authored-by: Yorke Rhodes <[email protected]>
### Description - Removed contracts-bedrock contracts dependency from eth-optimism (which was forcing us to use 0.8.15) - Added local minimal interfaces for the CrossDomainMessenger contract ### Drive-by changes None ### Related issues None ### Backward compatibility Yes ### Testing Unit tests --------- Signed-off-by: -f <[email protected]> Co-authored-by: Yorke Rhodes <[email protected]>
yorhodes
requested review from
jmrossy,
tkporter,
aroralanuk,
Skunkchain,
nambrot and
avious00
as code owners
nambrot
reviewed
Sep 15, 2023
### Description - Adding "InsertedIntoEvent" for standalone indexing of events by the agents ### Drive-by changes None ### Related issues #2313 ### Backward compatibility Yes ### Testing Unit tests
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2733 +/- ##
==========================================
- Coverage 67.91% 67.57% -0.34%
==========================================
Files 95 101 +6
Lines 1446 1024 -422
Branches 189 106 -83
==========================================
- Hits 982 692 -290
+ Misses 457 293 -164
- Partials 7 39 +32
|
### Description - Adds destinationDomain to the `GasPayment` event - recording destination domain as `destination` while reading events for the relayer ### Drive-by changes - none ### Related issues - fixes hyperlane-xyz/issues#475 ### Backward compatibility No, change in event emitted and relayer indexing ### Testing - Unit --------- Signed-off-by: -f <[email protected]> Co-authored-by: Yorke Rhodes <[email protected]>
Co-authored-by: Kunal Arora <[email protected]> Fixes hyperlane-xyz/issues#559
### Description - Consolidating IGPMetadata and OPStackMetadata to versioned GlobalHookMetadata - Add `supportMetadata` to `IPostDispatchHook` - Metadata Schema [0:1] variant [2:33] msg.value [34:65] Gas limit for message (IGP) [66:85] Refund address for message (IGP) [86:] Custom metadata ### Drive-by changes None ### Related issues - fixes hyperlane-xyz/issues#610 ### Backward compatibility No ### Testing Unit tests --------- Co-authored-by: Yorke Rhodes <[email protected]>
tkporter
reviewed
Sep 21, 2023
### Description Add deployer for routing and fallback routing hooks ### Drive-by changes None ### Related issues ### Backward compatibility Yes ### Testing Manual
### Description - key funder support to scroll and polygonZkEVM ### Drive-by changes ### Related issues - hyperlane-xyz/issues#656 ### Backward compatibility Yes ### Testing Manual
### Description - Deploys mainnet3 (and removes mainnet2) ### Drive-by changes - Changes protocol fees to 0 - Changes owners to deployer key (temporarily) ### Related issues - closes hyperlane-xyz/issues#655 ### Testing Some v2 related tests broken --------- Co-authored-by: Yorke Rhodes <[email protected]> Co-authored-by: Nam Chu Hoai <[email protected]>
Updates the images for testnet4 kathy that was borked from #2833
Co-authored-by: J M Rossy <[email protected]> Co-authored-by: Nam Chu Hoai <[email protected]> Co-authored-by: Kunal Arora <[email protected]>
…t` (#2861) ### Description - Removed the double mapping logic from the the `merkle_tree_builder.get_proof()` for fixing the following issue: > `get_proof` was expecting a nonce which it then uses to retrieve the `leaf_index` i.e. nonce -> message_id -> leaf_index and then `prove_against_previous` but in our case, we already got the leaf_index in merkle_tree_multisig.rs so what we end up doing is trying to look up the leaf_index twice and either not finding the message_id or proving wrong leaf_index which both causes the "cannot fetch metadata" for the merkle tree builder ### Drive-by changes - additional logging for aggregation to indicate which ism (moduleType, address) we are missing - trace -> debug logging for `sign_and_submit_checkpoint` ### Related issues - closes hyperlane-xyz/issues#695 ### Backward compatibility Yes ### Testing e2e --------- Co-authored-by: -f <[email protected]> Co-authored-by: Kunal Arora <[email protected]>
Co-authored-by: Yorke Rhodes <[email protected]> Fixes #2757
### Description PR adjusts the bytecode checking for V3 mailbox's ### Drive-by changes Adds a violation when a contract is not proxied, but be expected to be ### Backward compatibility <!-- Are these changes backward compatible? Are there any infrastructure implications, e.g. changes that would prohibit deploying older commits using this infra tooling? No --> ### Testing <!-- What kind of testing have these changes undergone? Manual -->
- Combine tx amounts in cw adapter when denoms match - Update CosmosTokenAdapters with latest from warp ui - Fix IBC-to-warp adapter for IGP support - Add logos for cosmos related chains
Co-authored-by: Yorke Rhodes <[email protected]>
### Description When no gas enforcement policy is set, by default, we should apply None --------- Co-authored-by: Trevor Porter <[email protected]>
Co-authored-by: J M Rossy <[email protected]> Fixes hyperlane-xyz/issues#687 Fixes #2228 Fixes hyperlane-xyz/issues#704
### Description - Merge latest main into v3, including notably the new CLI package. - Update CLI code for V3 ### Drive-by changes Housekeeping of package.json files ### Related issues Fixes #2811 ### Testing Manual and CI test updates --------- Co-authored-by: Kunal <[email protected]> Co-authored-by: Yorke Rhodes <[email protected]>
### Description - if the default multisig config contains the config for the chain we want to deploy on and we've not explicitiy provided a config, the CLI will use the default config ### Drive-by changes - ignoring the ismType in `multisig-ism.yaml` and deploying messageIdMultisig for all multisig configs provided indiscriminately. Also, added a note regarding this in the `example/multisig-ism.yaml` file. (otherwise we end up using `MultisigIsmConfig` and `MultisigConfig` interchangeably like in the `buildIgpConfig` which leads to confusion) **NOTE**: this doesn't affect our inability to not deploy the same multisig with different types for different remote chains because the config provided is origin specific, eg, you cannot use goerli multisig as messageId for arbitrumsepolia and for scrollsepolia as merkleRoot. - moved `buildMultisigIsmConfigs` to sdk as an effort to dedupe the two variations in infra and cli. ### Related issues - aimed at hyperlane-xyz/issues#530 (comment) ### Backward compatibility Yes ### Testing Manual --------- Co-authored-by: Trevor Porter <[email protected]> Co-authored-by: Daniel Savu <[email protected]> Co-authored-by: J M Rossy <[email protected]> Co-authored-by: Nam Chu Hoai <[email protected]> Co-authored-by: Rohan Shrothrium <[email protected]> Co-authored-by: Rohan Shrothrium <[email protected]> Co-authored-by: Mattie Conover <[email protected]> Co-authored-by: Yorke Rhodes <[email protected]>
### Description - Use default config paths in more places - Reorder some command args for clarity in help text - Bump versions to 3.1.3
### Description - Bring BigNumber de-duping improvements from @ottbunn into v3 - Fixes for amount conversion - Create unit tests for some amount fns - Run utils unit tests in CI ### Related issues hyperlane-xyz/issues#690 #2751 #2828 ### Backward compatibility No, minor breaking changes to signatures of amount util fns ### Testing Unit tests
Co-authored-by: Yorke Rhodes <[email protected]> Co-authored-by: J M Rossy <[email protected]>
daniel-savu
approved these changes
Nov 16, 2023
jmrossy
reviewed
Nov 16, 2023
aroralanuk
approved these changes
Nov 16, 2023
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+117
to
+121
| function claim() external { | ||
| // Transfer the entire balance to the beneficiary. | ||
| (bool success, ) = beneficiary.call{value: address(this).balance}(""); | ||
| require(success, "IGP: claim failed"); | ||
| } |
Check failure
Code scanning / Slither
Functions that send Ether to arbitrary destinations
|
|
||
| function _getConfiguredHook( | ||
| bytes calldata message | ||
| ) internal view virtual returns (IPostDispatchHook hook) { |
Check notice
Code scanning / Slither
Local variable shadowing
| mapping(uint32 => mapping(bytes32 => address)) public customHooks; | ||
|
|
||
| constructor( | ||
| address mailbox, |
Check notice
Code scanning / Slither
Local variable shadowing
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.
Description
Merges v3 feature branch to main
Backward compatibility
No
Testing
All