Skip to content
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

update wrap() to accept an amount parameter #48

Open
devtooligan opened this issue Aug 17, 2022 · 0 comments
Open

update wrap() to accept an amount parameter #48

devtooligan opened this issue Aug 17, 2022 · 0 comments

Comments

@devtooligan
Copy link
Contributor

Update wrap() to accept an amount parameter then be more deliberate about the amounts we are wrapping. We may want to update unwrap() as well for consistency. We may want to only update the internal methods, not the external.

here is bruno's pr where he first mentions this.

Here is a copy paste from slack:

update on yieldspace-tv
Marco and I met with Bruno this morning. Bruno had some concerns around retrieveBase as well the way we wrap/unwrap tokens.

One thing that Bruno pointed relates to repaying a vault, the steps are:

  • we send over required base (plus a bit extra for slippage) to the pool first
  • we call repayVault on the ladle, (which calls pool.buyFYToken )
  • within buyFYToken the first step is that we wrap all base found in the contract, then it does the swap. in the end there is no extra base in the contract, but there could be some leftover shares (i think this logic is pretty well understood by everyone at this point, we’ve had numerous discussions about it and has been reviewed/audited extensively)
  • the last thing the repayVault function does is call retrieveBase which in actuality does nothing , since any extra base in the contract has been converted to shares (this is the retrieveBAse that Bruce hit when he drained the pool)

Bruno proposed changing the way we wrap/unwrap. The crux of the proposed change is to add an amount param to the wrap fn and be more intentional when wrapping, as opposed to wrapping all base found in the contract. This would allow for the retrieveBase to work as intended in the repayVault fn as well as helping contango in some of their functions.

I think it sounds like a fine idea. I am concerned that we keep adding things at the last minute like this but I think we could safely land this change without too much delay in our ongoing push. Also, want to help out the Contango team. Plus this change may optimize the code a bit, and could (possibly) address one of the off-by-1-wei issues we’ve been having. I guess the alternative is do nothing and only update the ladle code but i think its worth at least seeing what this change looks like.

Unless anyone has any objections, I will create a new branch with the proposed changes and put it up for comment this afternoon.

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

No branches or pull requests

1 participant