Skip to content

Conversation

@crazybuster
Copy link
Contributor

@crazybuster crazybuster commented Aug 23, 2022

  • adds withdrawFromStrategy and depositToStrategy funds allocation utility functions
  • unit & fork tests added

Issue: #937

TODO (important):

  • we need to deploy this -> create deploy issue/PR

@micahalcorn micahalcorn removed the request for review from sparrowDom December 6, 2022 03:03
@sparrowDom sparrowDom self-assigned this Dec 6, 2022
@sparrowDom sparrowDom requested a review from shahthepro December 6, 2022 16:07
@sparrowDom
Copy link
Member

@shahthepro @DanielVF @franckc this is ready to be reviewed

Copy link

@franckc franckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a pass. A few small nits inline otherwise looks great.
Please take a look at the fork tests failures - they seem relevant.

@sparrowDom
Copy link
Member

with the latest commit only 1 unrelated Morpho fork test is failing.

@sparrowDom
Copy link
Member

@DanielVF a friendly ping

@franckc
Copy link

franckc commented Jan 19, 2023

Friendly ping @shahthepro @DanielVF for the review - not urgent at all but now that we have more strategies it would be nice to have that to save on gas and simplifies our weekly allocations.

@sparrowDom as part of this PR, do we also need to update any tooling (like scripts) that we use for generating the allocation transactions?

Copy link
Collaborator

@shahthepro shahthepro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, LGTM

@sparrowDom
Copy link
Member

good thinking @franckc! I think @DanielVF uses the brownie allocations.py to build the strategist allocations. @DanielVF do you want me to do the changes there as well, or will you do them as a part of next strategist action?

@sparrowDom sparrowDom force-pushed the yupan/withdraw-batch branch from 4c412e3 to e704bf6 Compare January 20, 2023 15:42
@DanielVF DanielVF changed the title Withdraw batch Move funds directly in and out from strategies Jan 25, 2023
@DanielVF DanielVF changed the title Move funds directly in and out from strategies Reallocate by moving funds directly between vault and strategies Jan 25, 2023
@DanielVF
Copy link
Contributor

Once we've used this in production for a few weeks, we'll probably want to remove the reallocate function. The gas savings is three transfers vs using deposit withdraw, but in the actual practice with our funds allocations, I don't think it would get used very often. Having two almost duplicate ways to do the same thing in the code isn't a good thing.

We won't make a change for this in this PR though.

Copy link
Contributor

@DanielVF DanielVF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed these changes, and they are good.

@DanielVF DanielVF merged commit a0902b7 into master Jan 30, 2023
@DanielVF DanielVF deleted the yupan/withdraw-batch branch January 30, 2023 14:10
@franckc franckc mentioned this pull request Feb 2, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants