AUTO-10716: fix a funding bug in LinkAvailableBalanceMonitor #13364
Conversation
| if (success) { | ||
| localBalance -= contractToFund.topUpAmount; | ||
| emit TopUpSucceeded(targetAddress); | ||
| emit TopUpSucceeded(targetAddress, contractToFund.topUpAmount); // which address should be emitted?? |
There was a problem hiding this comment.
@RyanRHall @wentzeld what do u think of this address? the proxy address or the aggregator (the actual address which gets the LINK)?
and what do you think if we add the top-up amount in the event?
There was a problem hiding this comment.
I like both those ideas :)
|
Some comments |
| } else { | ||
| s_targets[targetAddress].lastTopUpTimestamp = contractToFund.lastTopUpTimestamp; | ||
| s_targets[targetAddress].lastTopUpTimestamp = contractToFund.lastTopUpTimestamp; // should we update this? |
There was a problem hiding this comment.
@RyanRHall what do you think of this?
There was a problem hiding this comment.
this is super confusing and unnecessarily complicated lol. From what I can tell, the lastTopUpTimestamp is always updated on line 269, and then on line 283 and 287 we set the time stamp back to what it was (meaning net no change) - is that how you interpret this code too?
I think we should take a second and think about what makes most sense from a product perspective. The min wait period is supposed to act as a rate limiter. And I think the question we're asking here is "should the rate limit apply to failed transfers?". So imagine a transfer fails for some reason, do we want the system to retry immediately or only retry after some wait period? I would think the latter, but we might want to double check with finance team.
|
| .withArgs(aggregator2.address, twoLINK) | ||
| await expect(tx) | ||
| .to.emit(labm, 'TopUpSucceeded') | ||
| .withArgs(proxy3.address) | ||
| .withArgs(aggregator3.address, twoLINK) |
No description provided.