search

Swivel-Finance / gost

Smart contract testing with Geth via the Golang ABIGEN
20 stars 2 forks source link

Revisit the approvals for the lender and redeemer #348

Closed sourabhmarathe closed 2 years ago

sourabhmarathe commented 2 years ago

We should make sure that the principal tokens are correctly approved in the contracts.

Previously, we had the redeemer max-approved for all principal tokens (via the createMarket method in the MarketPlace contract).

The current model for custody has the lender holding the principal tokens, and the redeemer burning the equivalent amount of zc tokens when Illuminate's redeem is called. As a result, we should approve lender's spending of the principal tokens (perhaps making marketplace aware of lender's address?). (@JTraversa: would love your confirmation that this is the right idea)

This may also give us a chance for a slight refactor where we break out the Illuminate redeem into a separate method -- this would make sense as it is executing the final redemption & burn whereas the other redeem calls redeem PTs from their native protocols. This would improve the legibility of the code and better reflect the flow of custody in Illuminate more broadly. (Curious for your thoughts @robrobbins).

This is a lot of text, but these changes overall are minor.

robrobbins commented 2 years ago

If you think separating the illuminate redeem solves some issue then ofc i'm open to see it. my question would be is the signature changed or are you thinking of delegating out to an internal illuminate specific one from the parent overriden redeem?

sourabhmarathe commented 2 years ago

If you think separating the illuminate redeem solves some issue then ofc i'm open to see it. my question would be is the signature changed or are you thinking of delegating out to an internal illuminate specific one from the parent overriden redeem?

I don't think it solves an issue. Instead, I think the Illuminate redeem is fundamentally different from other principal's redeem calls. The Illuminate redeems funds to the user whereas the principal redeems funds to the redeemer address, which in turn can later be redeemed by the user by making a call to illuminate's redeem.

I think instead of breaking this off into another method, we should keep it as is and make sure that this fact is well documented (I will do this as part of this ticket).

JTraversa commented 2 years ago

I kind of agree with Sourabh that it is different in a way because the illuminate redeem is the only one that most users will likely ever call?

You could also maybe argue that some would redeem through the EIP-5095 interface instead anyways, and its an argument not to separate further? I'll let you guys really decide how to go on that end, I like separation, but i can see why you wouldnt.

On approvals:

As a result, we should approve lender's spending of the principal tokens (perhaps making marketplace aware of lender's address?)

I'm a bit lost in the sauce in exactly all this.

  1. Why does the lender spend PTs? It owns them, and the redeemer spends them?
  2. Where would the marketplace come in this, doesnt it just store the market itself / not have any custody?

Beyond all that, the approvals weve noted are necessary so far:

  1. External PT: Approve redeemer.sol to spend all of lender.sol's external PTs
  2. Underlying: Approve integrated protocol to spend all of lender.sol's underlyings 2a. Approve Aave to spend all of lender.sol's underlyings
  3. Aave Token: Approve APWine to spend all of lender.sol's Aave tokens

I cant think of any others?

sourabhmarathe commented 2 years ago
  1. External PT: Approve redeemer.sol to spend all of lender.sol's external PTs

Ok, this sounds good. We already have this in place in MarketPlace.sol

  1. Underlying: Approve integrated protocol to spend all of lender.sol's underlyings 2a. Approve Aave to spend all of lender.sol's underlyings

We can it outside of the contract or provide Aave token addresses to the createMarket function. Which would be preferred?

  1. Aave Token: Approve APWine to spend all of lender.sol's Aave tokens

I believe the plan is to call this externally as MarketPlace is not aware of lender's address.

JTraversa commented 2 years ago

Ok, this sounds good. We already have this in place in MarketPlace.sol

How? The approval always uses the msg.sender context of the address calling it, so Marketplace cant force lender.sol to approve marketplace.sol, it can only handle its own approvals so we'll need an approval method in lender.

We can it outside of the contract or provide Aave token addresses to the createMarket function. Which would be preferred?

Lets not make createMarket too heavy. + a lot of the time we will already have approved stuff. E.g. underlying -> protocol will only need to be done once.

I believe the plan is to call this externally as MarketPlace is not aware of lender's address.

We resolved this in dms but yeah marketplace doesnt need lender's address 👍