improve(SpokePool): Emit FundsDeposited event directly #249
Conversation
Stack-related concerns are hopefully mitigated by the recent use of an IR during contract compilation.
| fillCounter[token] += _computeAmountPostFees(totalFillAmount, realizedLPFeePct); | ||
| } | ||
|
|
||
| // The following internal methods emit events with many params to overcome solidity stack too deep issues. |
There was a problem hiding this comment.
Have removed this comment because I left _emitFillRelay() as a convenience. My thinking is that since it dereferences a lot of members of relayExecution and is called from multiple other functions, it's more practical to keep it as-is.
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
Seems reasonable! But, generally, we aren't supposed to make additions after the audit starts. Thoughts on leaving this out? Or at least asking OZ if they can review as a part of the fixes before merging?
I haven't quantified the gas impact of the change; I guess it's very small, though it is one of the most heavily used functions. I think it comes down to OZ and how they're tracking with the existing audit work. If they've got capacity to spare then it might make sense to ask if it can be considered, or perhaps add it as part of any follow-up changes that might be needed following their initial findings. I expect it's a fairly easy change for them to review, but if things are tight then we should leave it out. |
nicholaspai
left a comment
nicholaspai
left a comment
There was a problem hiding this comment.
I think we should merge it and include in list of small PR's that we'd like reviewed in post-fix audit period. I think this will improve readability and have small gas savings
Stack-related concerns are hopefully mitigated by the recent use of an IR during contract compilation.