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

feat: add an AMO minter and an Aave AMO #956

Merged
merged 43 commits into from
Oct 3, 2024

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Sep 7, 2024

Resolves #611

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Sep 7, 2024

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Refer to our other smart contracts for code styles.

Never reference token symbols as identifiers. Besides we already renamed uad to uusd, causing developer confusion down the road.

uAD -> Dollar
UBQ -> Governance

@zugdev
Copy link
Contributor Author

zugdev commented Sep 10, 2024

  • @0x4007 @rndquu can I get some feedback? I will start writing tests for the AMO minter and for the Aave AMO, therefore any core feedback would be invaluable until then. Regarding code style I will adhere to it as a last step, don't worry about it.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

I'm not a solidity developer, but nothing really stands out except for my one comment

packages/contracts/src/dollar/libraries/TransferHelper.sol Outdated Show resolved Hide resolved
@rndquu
Copy link
Member

rndquu commented Sep 10, 2024

  • @0x4007 @rndquu can I get some feedback? I will start writing tests for the AMO minter and for the Aave AMO, therefore any core feedback would be invaluable until then. Regarding code style I will adhere to it as a last step, don't worry about it.

Hey, sorry for a late response, I'll check it today

@rndquu
Copy link
Member

rndquu commented Sep 10, 2024

@zugdev

Notes:

  1. AaveAMO doesn't really use UbiquityAMOMinter (although defines it in the constructor) so it's not clear how those 2 contract will interact with each other and UbiquityPool.
  2. UbiquityAMOMinter is basically a fork of FraxAMOMinter (which is expected). We need only methods related to collateral moving to and from AMO strategies.
  3. All aave related structs/interfaces/contracts should be taken from aave github repository (i.e. aave should be installed in github submodules) instead of copy/pasting them.
  4. There is no need in copy/pasting TransferHelper lib, just use SafeERC20.

Question:

@zugdev
Copy link
Contributor Author

zugdev commented Sep 10, 2024

  1. and 2. - was waiting for this specific clarification to resolve.

  2. and 4. - understood.

What contract did you use as a base one from the Frax repo for the AaveAMO

Used AaveAMO_V2

Yes, there is still some stuff to be tuned, will wrap it this week.

@zugdev
Copy link
Contributor Author

zugdev commented Sep 11, 2024

Yo it should be well directed now. I've:

  1. Refactored Aave AMO from V2 to V3, most liquidity is there.
  2. Implemented rewards collection mechanism.
  3. Using contracts from git submodule instead of copypasted interfaces.
  4. Refactored code to use collateral only.
  5. Implemented emergency mechanisms: arbitrary calls and ERC20 recovery for owner - your choice to keep.

The AMO has the minter address to return collateral. I have implemented borrow and repay as well, this could be kept or removed - its a trust decision.

I still have to write tests and addapt code style, will be pushing this when I get some free time.

A little more context on 5: to handle rewards, borrows and repays, the system would need some way to swap, could design a Uniswap AMO. There could already be something in place, I haven't checked if it exists as a facet.

@0x4007
Copy link
Member

0x4007 commented Sep 12, 2024

Cowswap is preferred if that's an option!

@zugdev
Copy link
Contributor Author

zugdev commented Sep 12, 2024

Cowswap is preferred if that's an option!

Can do that! Let me wrap this one first though.

@zugdev zugdev marked this pull request as ready for review September 18, 2024 18:02
@zugdev
Copy link
Contributor Author

zugdev commented Sep 18, 2024

@rndquu I've marked this PR as ready for review. Please let me know if it is up to your standards. I agree an issue to write a non-forked test suite is appropriate, I would be down to go through with it.

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Ok, here comes a list of code style refactors:

  1. The whole codebase uses camel cases naming so all snake case variables should be refactored to camel case
  2. There's also a convention across the whole code base to use camel case for abbreviations (like Nft not NFT). So all AMO related namings should be refactored to Amo. Examples:
  • receiveCollatFromAMO => receiveCollatFromAmo
  • validAMO => validAmo
  1. All UAD namings should be refactored to Dollar
  2. No shortages. Be as explicit as possible. Example:
  • receiveCollatFromAMO => receiveCollateralFromAMO

Regarding the code:

  • TransferHelper lib should removed and SafeERC20 should be used. Otherwise we'll have to perform a new audit contest solely for the copy/pasted lib.

packages/contracts/src/dollar/interfaces/IAMO.sol Outdated Show resolved Hide resolved
@zugdev zugdev requested a review from rndquu September 20, 2024 02:05
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Overall this PR looks good:

  • natspecs everywhere
  • unit tests make sense
  • owner seems to able to transfer collateral to aave

There're a couple of things that we'll have to work out (not part of this PR):

  • remove forked tests in favor of unit tests
  • AmoMinter owner should be able to transfer tokens to aave in a single tx (right now owner has to execute a 2nd tx to deposit collateral to aave)
  • IAmo could have deposit() and withdraw() methods (which fits at least with lending and DEX protocols)

I will review this PR in detail on Monday. Meanwhile pls fix minor issues in the comments.

.gitmodules Show resolved Hide resolved
packages/contracts/src/dollar/amo/AaveAmo.sol Outdated Show resolved Hide resolved
packages/contracts/src/dollar/amo/AaveAmo.sol Outdated Show resolved Hide resolved
@zugdev
Copy link
Contributor Author

zugdev commented Sep 21, 2024

  • AmoMinter owner should be able to transfer tokens to aave in a single tx (right now owner has to execute a 2nd tx to deposit collateral to aave)

There are a couple ways to do that. Adding AMO contracts to the main diamond or a specific AMO diamond is one of them. If so the IAmo concept will be extinguished, as function names can't be shared.

  • IAmo could have deposit() and withdraw() methods (which fits at least with lending and DEX protocols)

Another oposing idea could be adhering to this IAmo idea, keep standalone AMO contracts and implement overriden deposit and withdraw methods so that function giveCollateralToAmo receives a fixed set of arguments. I am against this approach as the arguments passed in depositing/withdrawing would become static, limiting a lot possivle strategy branches - as different deposit functions require different arguments. But it is indeed an option.

@zugdev zugdev requested a review from rndquu September 24, 2024 19:17
@zugdev zugdev changed the title Add an AMO minter and an Aave AMO feat: add an AMO minter and an Aave AMO Oct 1, 2024
@rndquu rndquu requested review from rndquu and removed request for rndquu October 1, 2024 09:06
@rndquu rndquu mentioned this pull request Oct 1, 2024
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

This PR implements basic functionality for the AMO minter and aave AMO. I've opened another issue for polishing this PR and preparing deployment migrations.

Then we could utilize https://www.findaudit.xyz/ or ask 0xAdrii to look though the contracts.

@0x4007 FYI

@rndquu rndquu requested a review from 0x4007 October 1, 2024 09:33
@rndquu rndquu merged commit 386de2a into ubiquity:development Oct 3, 2024
28 checks passed
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.

Algorithmic Market Operations for Collateral
3 participants