- 
                Notifications
    
You must be signed in to change notification settings  - Fork 13
 
feat: Update contract #108
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
Conversation
| 
           Closing in favor of #130  | 
    
c33f0f4    to
    4d8d4ce      
    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.
A couple of comments about messaging. I'll go deeper into the logic later
This reverts commit ed20ca6.
9208af1    to
    17f7a92      
    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.
Despite the amount of comments, I think the main logic is correct! Good job here 🚀
| IAuth(tranche.token).rely(manager); | ||
| escrow.approveMax(tranche.token, manager); | ||
| escrow.approveMax(asset, manager); | 
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.
Question. What's the difference here between token and asset?
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.
asset -> e.g. USDC
token -> e.g. LTF
| // TODO: Check whether to check against asset of vault in storage?? | ||
| return _vaultToAsset[vault].isLinked; | 
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.
Reminder here to close it before merge.
I think it makes sense as it is (?)
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.
@mustermeiszer Can I remove the TODO comment in #98?
        
          
                src/vaults/PoolManager.sol
              
                Outdated
          
        
      | function updateContract(uint64 poolId, bytes16 trancheId, address target, bytes memory update_) public auth { | ||
| if (target == address(this)) { | ||
| (bool success, bytes memory data) = address(this).delegatecall( | ||
| (bool success, ) = address(this).delegatecall( | 
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.
It's interesting to return the error from data in case it fails. You can check how Multicall.sol does it.
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.
Done
| function rely(address user) public auth { | ||
| wards[user] = 1; | ||
| emit Rely(user); | ||
| } | ||
| 
               | 
          ||
| /// @inheritdoc IAuth | ||
| function deny(address user) external auth { | ||
| function deny(address user) public auth { | 
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 think these can be external again.
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.
No, the investment manager is still using them from this
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.
Could you link me to that line?
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.
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.
Ok, I see, later the vault needs to call manager 👍🏻.
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.
LGTM! Great work 🚀
* fix pools ITs and remove allow/disallow * feat: add new message type * feat: add IUpdateContract * wip: link and unlink vaults * wip: final iteration for now * Revert "fix pools ITs and remove allow/disallow" This reverts commit ed20ca6. * wip: add length, safeguard against unknown vault implementations * chore: review stuff * chore: fmt world * wip: compile till tests * wip: test compile... * chore: compile * wip * fmt * fix: test correct encoding of updateContract * fix: array access * fix: tests * wip: disable rounding test failure * chore: fmt * feat: submessages * feat: remove need for rely on self * feat: delefageCall and vault change * fix: test with latest foundry * chore: fmt nighlty * chore: comments * chore: add rerror handling to delgeate all PM * wip: try reduce code-size * wip: try reduce code-size --------- Co-authored-by: lemunozm <[email protected]>
Introduces a message
UpdateContractthat allows for the CP side to call into any other contract on the CV side that implementIUpdateContract.This feature is then used in the
CV.PoolManagertoFollow-Up
Dis/AllowAssetmessage flow will be done as part of feat: ERC-6909 Compatibility #98