Adapt the contract to simplify UBQ farming

sergfeldman commented 2 years ago

Background

The source code in this repo https://github.com/ubiquity/ubiquity-instadapp/tree/master/backend/contracts/mainnet

We made this contract for Instadapp https://etherscan.io/address/0x8ec066d75d665616a94f2eccdbe49b54eaeefc78#code

To do

Adapt the contract to simplify UBQ farming

Adapt the contract for Instadapp for the Ubiquity website to simplify liquidity providing for DeFi amateurs. The first step of this assignment is to adapt zapaz's code (remove all the instadapp specific code) to allow users to directly farm from our UI.
The second part will be eventually to make a UI component to operate this smart contract.

Acceptance Criteria

TBD

0x4007 commented 2 years ago

For some additional context, the current process to farm UBQ requires the user to follow several steps and navigate to several UIs. https://dao.ubq.fi/how-to-start-farming-ubq

The instadapp "ape in" code that zapaz made consolidates the steps into a single transaction, which will make it easier for users to start farming. The first step of this assignment is to adapt zapaz's code (remove all the instadapp specific code) to allow users to directly farm from our UI.

The second part will be eventually to make a UI component to operate this smart contract.

kfujita0520 commented 2 years ago

Please, assign this to me. I would like to try. By the way, when I tried to set up this module as instructed in readMe, "yarn install" failed on backend part with following error error An unexpected error occurred: "https://registry.yarnpkg.com/@noble/secp256k1/-/secp256k1-1.5.1.tgz: Request failed \"404 Not Found\"". Any configuration issue? Kindly please advise.

sergfeldman commented 2 years ago

@kfujita0520 Assigned to you. Please explore this issue yourself. If you can’t figure it out, please put comments what you tried and what results it gave.

0x4007 commented 2 years ago

Please, assign this to me. I would like to try. By the way, when I tried to set up this module as instructed in readMe, "yarn install" failed on backend part with following error error An unexpected error occurred: "https://registry.yarnpkg.com/@noble/secp256k1/-/secp256k1-1.5.1.tgz: Request failed "404 Not Found"". Any configuration issue? Kindly please advise.

If CI/CD works on the commit you're working off of then its an issue on your end. I recommend updating to the latest commit on development and verifying that CI/CD passed, then try running again.

sergfeldman commented 2 years ago

@kfujita0520 Please, share updates on solving this issue by Monday 9/19/2022

kfujita0520 commented 2 years ago

Sorry for being pending for a while. As reported I had an issue to process at initial stage when executing yarn install command. By removing yarn.lock file i could move on. after configuring some of parameter such as private keys, alchemy api key, now i can run this app on forking environment. But to be honest, I would not really understand what I am expected to improve. Can someone kindly spend sometime for chat with me to explain the background? then i could moveon easier. I have messaged in discord a while ago, but could not get any reply. Thank you for your kind support in advance.

codeawayaks commented 2 years ago

You can assign this to me.

kfujita0520 commented 2 years ago

wait. would you mind give me a chance? i have make it workable on local environment. I just would like to have some quick chat on discord.

sergfeldman commented 2 years ago

@kfujita0520 Please, note that implementation already took way more time than expected. You have time to complete this issue by the end of September 2022.

kfujita0520 commented 2 years ago

@sergfeldman Thank you very much for your arrangement. So far i have just making existing product workable on local, read your specification doc and interact with your protocol in production. I did not understand what this assignment expected me to date. But fortunately today, I will be able to communicate with your team on telegram so that now I understand what i need to do. Kindly give me time. I will update the progress within a week.

kfujita0520 commented 2 years ago

Progress Update:

I have put serious effort in the last weekend and created the necessary contract in my own repository. The contract is tested and confirmed in a few basic scenario. https://github.com/kfujita0520/simplify-ubq-farming

TODO

Add event log information (and remove debug log)
Integrate this contract into your product. (Ex. fetch UBQ related address through your ubiquityManager contract)
Accommodate to your coding guideline. (Ex. use Foundry in test framework or LintES, comment regulation etc...which yet I catch up)

I will update the progress of above work within the week again. Meanwhile please review in basic level and feedback if my developed contract function is different from what you have expected.

Thank you very much.

0x4007 commented 2 years ago

Meanwhile please review in basic level and feedback if my developed contract function is different from what you have expected.

Thank you very much.

@0xcodercrane is the most suitable to take a look. I understand that he is on holiday this week, but let's see if we can spend some time reviewing before your next update. Thanks!

0x4007 commented 2 years ago

I'm not a Solidity developer, but I have plenty of development experience. Here's my feedback. Ideally if we were able to leave line-by-line reviews in a pull request, it would make it faster for me to articulate. For now, I'll try and explain here.

My intuition says that we should make getters/setters for all of these addresses. This allows us flexibility in the future to update the main pool, like to an official Curve one with gauge weights, for example.

Also in general I encourage you to rename variables to be generic. For example, in the core Ubiquity Dollar smart contracts, we renamed variables named uAD into dollarToken etc. This is because we technically have the ability to modify the token symbol. UBQ into governanceToken.

In regards to making USDT/USDC/DAI generic: are you able to store these tokens in an array and loop through them? I've seen Curve smart contracts implement in this sort of manner. That way, we are able to completely change the metapool and support different assets (e.g. LUSD, FRAX)

Also we are rebranding the Bonding contract into Staking. I feel that it would be less ambiguous as we roll out our Bonds program. Do not include versioning (e.g. V2)

https://github.com/kfujita0520/simplify-ubq-farming/blob/2d6417c9a25a79c5b8346c7d042fd8f3537a4402/contracts/DirectUBQFarmer.sol#L24-L32

Is the empty constructor necessary?

0xcodercrane commented 2 years ago

Would you like to create a pull request to development branch? @kfujita0520 .

sergfeldman commented 2 years ago

@kfujita0520 Thank you for the update.

We review only pull requests to our repositories.

kfujita0520 commented 2 years ago

@pavlovcik Thank you very much for your quick and kind comments.

My intuition says that we should make getters/setters for all of these addresses. This allows us flexibility in the future to update the main pool, like to an official Curve one with gauge weights, for example.

In solidity, as long as declared as public, caller can access to this field so that getter function is not necessarily. For setter, as spoken, I am planning to integrate with your existing ubiquityManager contract, so that i don't need to hardcode some of addresses here. Instead fetch it from your system. (you could update it whenever needed.)

Also in general I encourage you to rename variables to be generic. For example, in the core Ubiquity Dollar smart contracts, we renamed variables named uAD into dollarToken etc. This is because we technically have the ability to modify the token symbol. UBQ into governanceToken. In regards to making USDT/USDC/DAI generic: are you able to store these tokens in an array and loop through them? I've seen Curve smart contracts implement in this sort of manner. That way, we are able to completely change the metapool and support different assets (e.g. LUSD, FRAX)

Ok, I can do, but I am afraid by doing so, the source code will be less readable. When reading the code of Curve Finance, sometimes frustrated as it is not intuitively understandable the logic flow when we see their index number. (I must look for definition separately) If you have specific purpose on contract and do not have plan to generalize the use-case anytime soon, specific variable name might be more readable.(At least during initial development and test it makes easier :) ) but maybe I can separately create generalized contract and set specific token address at constructor. That is perhaps what you would like to have?

Also we are rebranding the Bonding contract into Staking. I feel that it would be less ambiguous as we roll out our Bonds program. Do not include versioning (e.g. V2)

Noted.

Is the empty constructor necessary?

At the moment. No. I planed to pass the address of your ubiquityManager in the next version. but I should have commented so.

@0xcodercrane @sergfeldman

Would you like to create a pull request to development branch?

Yes, I am planning to create pull request from forked branch. but to do so, I need to integrate my code into your existing code more tightly and follow your coding standard, right? Plus, I think you expect me to write test case in Foundry? Since I am not catching up those rules yet, I need some more time to make pull request ready. I just would like your team if general direction of my work is correct or wrong. (More like casual conversation on code level) Anyway, thank you very much for your feedback. I will update the progress within a week again.

0x4007 commented 2 years ago

Ok, I can do, but I am afraid by doing so, the source code will be less readable. When reading the code of Curve Finance, sometimes frustrated as it is not intuitively understable the logic flwo when we see their index number. (I must look for definition separatelly) If you have specific purpse on contract and do not have plan to genralize the usecase anytime soon, specific variable name might be more readable.(At least during initial development and test it makes easier :) ) but maybe I can separatelly create generalied contract and set specific token address at constructor. That is perhaps what you would like to have?

Readability is great to strive for but we have plans to change our collateral to more censorship resistant assets (like LUSD, RAI) some research is still being done to decide the final basket. For the near future we should continue to support the 3pool (DAI/USDC/USDT)

Comments in the code should bandaid the readability for now.

kfujita0520 commented 2 years ago

Update: I spend quite some time on weekend, but not significant update to show this time as code base. Since I am new to Foundry, I have set it up and create simple test script for that. However, I am struggling. With Foundry, I cannot really interact with CurveFi protocol. (will result in revert error.) although I can do so with other protocol like Uniswap. In detail, CurveCryptoSwap exchange function cause revert error. If your team has insight on it, your advice is highly appreciated.

For integration part, I am also working on, but not tested yet. I mistakenly made pull request (into wrong branch), since it is still working in progress, please reject. Sorry and thank you very much

0x4007 commented 2 years ago

Since I am new to Foundry, I have set it up and create simple test script for that. However, I am struggling.

You might have a better time by asking your Foundry questions in the DevPool chat. If it ends up being a complex fix, I'm sure we'll naturally break off the conversation to a private group chat. Hope that will help!

0x4007 commented 1 year ago

@kfujita0520 looks like we forgot to label the price this bounty. According to the pricing table this should be $500.

What is your DAI wallet address? Also I just realized that another bounty hunter finished the tests. We need to decide internally on how to best handle this compensation.

0x4007 commented 1 year ago

@ryzhak I want to give you a one-off bonus for your speedy coding this last week and willingness to jump in and help @kfujita0520. Could you also share your DAI wallet address here?

ryzhak commented 1 year ago

@pavlovcik

wallet: https://etherscan.io/address/0x165343c045a00770b61DbCa01956590cC6005666

0x4007 commented 1 year ago

https://etherscan.io/address/0x165343c045a00770b61DbCa01956590cC6005666

https://etherscan.io/tx/0xacf0a9d66ffe4578f454a0292ce62577f042930ae1a0a4a3d12db0fb5ca6d28b https://etherscan.io/tx/0x345eb02b08b695951c92face909925da68b8a5ad29ffe116c851e4c1c5369b20

kfujita0520 commented 1 year ago

@pavlovcik Thank you very much. Here goes my wallet address. 0xA683026c4E5fe99192b0F209CAE3A3df3ebFa38D

0x4007 commented 1 year ago

0xA683026c4E5fe99192b0F209CAE3A3df3ebFa38D

https://etherscan.io/tx/0xb59ad98ed742c259b55b5d2452552855e8337650e502f7e105ca21763437dded

ubiquity / ubiquity-dollar