feat: release 0.4.4 #542
feat: release 0.4.4 #542
Conversation
* refactor: reduce base strategy size * fix: do not add onlyManagement
* fix: allow harvest on add strategy block * docs: comment Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
* fix: implement modifiers * fix: typo * fix: formatting
|
@storming0x |
yeah this one seems to match what we have on the branch here yearn-vaults/contracts/BaseStrategy.sol Line 736 in 887d909 Also diff here from the 0.4.3-sol8 branch vrs BaseStrategy here https://www.diffchecker.com/Su6r0fw9 LGTM! |
|
Do not merge into master yet. |
| } | ||
|
|
||
| function _onlyAuthorized() internal { | ||
| require(msg.sender == strategist || msg.sender == governance()); |
There was a problem hiding this comment.
I would keep the error messages in the require statements.
There was a problem hiding this comment.
Strategies have some issues with gas limit, I don't think it's necessary to get an error message.
There was a problem hiding this comment.
any reason not to swap over to using Custom Errors as you've moved to solc > 8.4?
contracts/Vault.vy
Outdated
| @@ -221,6 +233,7 @@ emergencyShutdown: public(bool) | |||
|
|
|||
| depositLimit: public(uint256) # Limit for totalAssets the Vault can hold | |||
| debtRatio: public(uint256) # Debt ratio for the Vault across all strategies (in BPS, <= 10k) | |||
| totalIdle: public(uint256) # Amount of tokens that are on the vault | |||
There was a problem hiding this comment.
*"on" should be come "in"
or change comment to "balance of underlying tokens tracked by the vault"
latest commit rollbacks to 0.3.3 FYI |
|
Associated PR: yearn/brownie-strategy-mix#88 |
|
can we squeeze this in? #545 |
* fix: change permit definition * fix: tests * chore: format
| @@ -741,7 +763,7 @@ def decreaseAllowance(spender: address, amount: uint256) -> bool: | |||
|
|
|||
|
|
|||
| @external | |||
| def permit(owner: address, spender: address, amount: uint256, expiry: uint256, signature: Bytes[65]) -> bool: | |||
| def permit(owner: address, spender: address, amount: uint256, deadline: uint256, v: uint8, r: bytes32, s: bytes32) -> bool: | |||
There was a problem hiding this comment.
from @xgambitox
Think it might break the interface that other integrators use, like Portals zaps, on withdrawing with permit. Take a look here https://etherscan.io/address/0x70ed999E2849A3C85EB4a6288B90c7ecA7b807F4#code#F9#L152
We probably need to either keep this interface or make a second permit that keeps backward compatibility to avoid breaking interface
There was a problem hiding this comment.
It's not possible to define multiple functions with the same name using vyper.
There was a problem hiding this comment.
I have made the changes, since optimism is already using 0.4.4, this might need to be pushed to 0.4.5. 1446bc4
There was a problem hiding this comment.
agree it makes sense to move to 0.4.5 to avoid confusion , so i guess next steps is release 0.4.4 and then 0.4.5 with this change
There was a problem hiding this comment.
I would like permit to be eip-2612 compliant and would instead use 0.4.4. The best would be for vyper to support multiple function definitions.
There was a problem hiding this comment.
As a hack, you can define the permit function to be 2612 compliant, and then farm another function name that has the same method ID as permit(address,address,uint256,uint256,Bytes[65]) and accepts the same type arguments, and then just have that split the signature into components and forward to the correct method
|
I will merge this into master and cut 0.4.4 release if none express any issues @storming0x @banteg @rareweasel |
asking in the protocol group so we are sure that we are all in sync w the plan |
Candidate for next release.
Do not merge until it's ready for a release.
Change Log:
closes #153