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: Algorithmic Market Operations for Collateral #848

Closed
wants to merge 12 commits into from

Conversation

FibrinLab
Copy link
Contributor

Resolves #611

Hello,
Forked from Frax and ERC 4626 compliant.
Increasing test coverage.
Comments Natspec.
Cannot get Ubiquibot to assign task to me.

Looking forward to review.
Thanks

@molecula451 molecula451 self-requested a review December 11, 2023 06:11
@molecula451
Copy link
Member

can you handle the changes, @FibrinLab ?

@FibrinLab
Copy link
Contributor Author

can you handle the changes, @FibrinLab ?

Yes. Working on it. Adding a few more tests to increase coverage

@molecula451
Copy link
Member

i must admit it's looking much better now

@molecula451 molecula451 self-requested a review December 11, 2023 20:19
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.

Code style changes. Check our contributions guidelines in this repo.

@FibrinLab
Copy link
Contributor Author

Code style changes. Check our contributions guidelines in this repo.

Its fixed

@molecula451
Copy link
Member

molecula451 commented Dec 12, 2023

nice but please refactor everything with underspace name_var_space, we use camelCase in the repo for naming @FibrinLab also it looks like the build it's failing

@ubiquity ubiquity deleted a comment from ubiquibot bot Dec 12, 2023
@ubiquity ubiquity deleted a comment from ubiquibot bot Dec 12, 2023
@ubiquity ubiquity deleted a comment from ubiquibot bot Dec 12, 2023
@gitcoindev
Copy link
Contributor

Hi, I will also review once the build is fixed

Compiler run failed:
Error (6275): Source "src/dollar/interfaces/IAmo.sol" not found: File not found. Searched the following locations: "/home/runner/work/ubiquity-dollar/ubiquity-dollar/packages/contracts".
  --> src/dollar/libraries/LibDollarAmoMinter.sol:10:1:

@0x4007
Copy link
Member

0x4007 commented Dec 12, 2023

nice but please refactor everything with underspace name_var_space, we use camelCase in the repo for naming @FibrinLab also it looks like the build it's failing

I recently added a ton of linter rules in @ubiquity/ts-template

Perhaps they should be migrated to this repository. Unfortunately they are only configured for TypeScript but maybe there's a plugin for it to work with solidity.

@molecula451
Copy link
Member

I recently added a ton of linter rules in @ubiquity/ts-template
Unfortunately they are only configured for typescript but maybe there's a plugin for it to work with solidity.

it does sound like a must-have implementation

@ubiquity ubiquity deleted a comment from ubiquibot bot Dec 12, 2023
@ubiquity ubiquity deleted a comment from ubiquibot bot Dec 12, 2023

This comment was marked as duplicate.

function dollarBalances()
external
view
returns (uint256 dollarValE18, uint256 collatValE18)
Copy link
Member

@molecula451 molecula451 Dec 14, 2023

Choose a reason for hiding this comment

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

you are doing good but this is unnaceptable, we can't have this naming, e.g dollarValE18

Copy link
Member

Choose a reason for hiding this comment

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

why do you want to refer to this naming like that? @FibrinLab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to stay consistent with the fork. I will just switch to simpler var names.

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.

@FibrinLab

There is the UbiquityPoolFacet contract where users can:

  1. Deposit collateral tokens (LUSD for example) and get Dollar tokens
  2. Redeem(burn) Dollar tokens in exchange for collateral

As a part of the current issue we should:

  1. Implement AmoMinter contract (example) which is responsible for transferring unused collateral from UbiquityPool to AMO contracts. There is already special method for borrowing collateral assets which can be called only by the AmoMinter contract.
  2. Implement 1 AMO strategy (example) and make it ERC-4626 compatible.

So in the end we should have a mechanism (set of contracts) where admin can transfer collateral tokens to one of the AMO strategies.

Right now AmoMinter and at least 1 AMO strategy contracts are not implemented.

@FibrinLab
Copy link
Contributor Author

  1. example

Thanks for the review. AMO minter is implemented. Adding a strategy.

@molecula451
Copy link
Member

we want to have this one too @FibrinLab , do not leave the PR stale, it's good you update us

@FibrinLab
Copy link
Contributor Author

we want to have this one too @FibrinLab , do not leave the PR stale, it's good you update us

Hello. Making final changes. Almost ready

@FibrinLab
Copy link
Contributor Author

@molecula451 Here is the initial draft. Adding tests and comments.

@gitcoindev
Copy link
Contributor

Hi @FibrinLab unfortunately this pull request seems to break a few CI checks. Could you please have a look at the failures and try to fix them or re-run if needed? The GitHub action results are available in above , I am pasting the links for reference:

Build & Test Details
Compare Test Coverage Details
Slither Analysis Details
Spell Check Details

@molecula451
Copy link
Member

molecula451 commented Dec 29, 2023

@FibrinLab please fix all the build erros, you can do yarn build:all to build and make sure all build pass before commiting changes

@rndquu rndquu marked this pull request as draft December 31, 2023 14:17
@molecula451
Copy link
Member

closing looks like the contributor lose interests in working on the issue

@molecula451 molecula451 closed this Feb 5, 2024
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
5 participants