-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Make LinkMonitor CLA compatible #11427
Conversation
I see that you haven't updated any README files. Would it make sense to do so? |
PTAL @RyanRHall, and also please @RensR take a look as the CCIP sme. |
58cc1c7
to
4d61e1f
Compare
4d61e1f
to
1a1f9ac
Compare
6532837
to
9ffe94a
Compare
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
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.
Missing a lot of test coverage
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
d6c69fe
to
64f4753
Compare
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 don't think the test changes quite match the functionality changes. Ex I don't see anything in the tests about the executor role? Also some new edge cases were added to addToWatchlist()
that should have test cases.
Also, this contract is very CCIP specific. I think you might want to either (A) make a copy of the original LinkAvailableBalanceMonitor
contract or (B) rename this to something like CCIPOnRampBalanceMonitor
or similar. I think preference for (A) as iirc this contract was supposed to serve any contract that implements the linkAvailableForPayment()
function (ex VRF) but these changes only make sense for CCIP.
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/automation/upkeeps/LinkAvailableBalanceMonitor.sol
Outdated
Show resolved
Hide resolved
Working on the unit tests, I'll add them asap. Regarding the CCIP specific part, I agree. The idea here is this contract will watch over every kind of contract (vrf, data feed proxies..) but also over CCIP onRamps, and in this last case it will be triggered automatically through the onRampSet event (using Automation). So that's why some of the logic has to be changed while it's still capable of dealing with any other contract type. |
1280e9f
to
878041d
Compare
* Introduce job spec flag for custom reverted pipeline * Disable the flag for V2+ * Rename file after merge conflict
SonarQube Quality Gate 0 Bugs No Coverage information |
addToWatchList
function has to be CL Automation compatible.removeFromWatchList
function has to properly clear the address from the watchList and set it as not active.