-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(contracts): InterchainAccountRouter minor audit remediation #4581
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4581 +/- ##
==========================================
+ Coverage 73.89% 73.93% +0.03%
==========================================
Files 100 100
Lines 1421 1427 +6
Branches 180 180
==========================================
+ Hits 1050 1055 +5
- Misses 350 351 +1
Partials 21 21
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my mental model for events is to surface data that is cumulative/not a constant time view call
dont feel strongly but this seems superfluous
I dont think we will ever build an indexer for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it would be quite simplifying if we just treated this like a normal router
theres not really a big cost of just deploying your own if you cant enroll on an existing set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but probably not the place to make this change
is hookset not something we can use in the validator if say the merkle hook gets updated in the future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all lgtm except the only initializing change, please revert
function setHook( | ||
address _hook | ||
) public override onlyInitializing onlyContractOrNull(_hook) onlyOwner { | ||
hook = IPostDispatchHook(_hook); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont agree that this should be onlyInitializing
_router, | ||
_body, | ||
new bytes(0), | ||
hook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good catch
Description
Drive-by changes
Related issues
Backward compatibility
No
Testing
Unit tests