Fixes on flashFee #14
Conversation
alcueca
commented
Aug 8, 2023
alcueca
commented
- flashFee returns type(uint256).max on lack of liquidity
- flashFee returns on unsupported assets (only managed for ERC3156Wrapper and Uniswap)
- flashFee returns on unsupported assets (partial)
| function maxFlashLoan(address asset) external view returns (uint256) { | ||
| return _maxFlashLoan(asset); |
There was a problem hiding this comment.
Why not making it public instead?
There was a problem hiding this comment.
Since calling a public from the same contract is an external CALL, I have the habit of using only internal and external, which also serves to segregate functionality and UX very clearly.
There was a problem hiding this comment.
Afaik That's not true, a call to a public function uses a JUMP when called from the same contract, unless you prepend this, in that case it would use a CALL.
Public functions have no extra overhead over internal ones
There was a problem hiding this comment.
Maybe things have changed in some recent version of solidity, and I didn't realize. Do you want to fix this through the wrappers, or to let it be?
There was a problem hiding this comment.
As I knew it, if you call an internal from the same contract is a JUMP, a public would be a CALL, and for an external you have to prepend this. and it will be a CALL as well.
There was a problem hiding this comment.
afaik that was never the case, the problem in older versions of solidity was that public functions couldn't use calldata params so they could be more expensive when calling as internal... there's an easy way to verify what I say, if it uses a JUMP, msg.sender wouldn't change, if it's a CALL, then msg.sender would be address(this), I'm 99.99999e18 sure that it's the former.
There was a problem hiding this comment.
Still, I like the separation and I'm not so keen on a refactor. Do you mind if we keep it as it is?
