Curve amm adapter #155

RedVeil · 2021-10-21T15:32:44Z

Code Review Processes

New Feature Review

Before submitting a pull request for new review, make sure the following is done:

Design doc is created and posted here: [Insert Link]
Code cleanliness and completeness is addressed via guidelines

README Checks

README has proper context for the reviewer to understand what the code includes, any important design considerations, and areas to pay more attention to

Code Checks

Add explanatory comments. If there is complex code that requires specific context or understanding, note that in a comment
Remove unncessary comments. Any comments that do not add additional context, information, etc. should be removed
[] Add javadocs. -- how do i do this?
Scrub through the code for inconsistencies (e.g. removing extra spaces)
Ensure there are not any .onlys in spec files

Broader Considerations

Ensure variable, function and event naming is clear, consistent, and reflective for the scope of the code.
Consider if certain pieces of logic should be placed in a different library, module

utils/contracts/curve.ts

bweick · 2021-10-21T21:03:36Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+        for (uint256 i = 0; i < 2; i++) {
+            if (underlying[i] == _token) return i;
+        }
+        revert("token not in pool");


Don't think we have coverage on this revert

Though this revert may not be necessary because of _isValidPool we know that the passed components exist in that Meta Pool otherwise Line 160 would resolve false

you are right, i deleted the revert

bweick · 2021-10-21T21:09:11Z

curveFactoryMetapoolAmmAdapter.md

@@ -0,0 +1,9 @@
+## Curve FactoryMetapool Amm Adapter


This is great context, lets put it in the javadocs in the adapter contract and remove this markdown file. We like to keep all the context in the contract itself.

bweick · 2021-10-21T21:12:04Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+import { IMetaPoolZap } from "../../../interfaces/external/IMetaPoolZap.sol";
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+
+contract CurveFactoryMetapoolAmmAdapter is IAmmAdapter {


If we can add some of the javadocs and section headers you see here for consistency.

bweick · 2021-10-21T21:15:30Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+        override
+        returns (address, uint256, bytes memory)
+    {
+        require(_isValidPool(_pool, _components), "invalid pool");


Don't need this because this check comes standard on the AmmModule, see here

true, deleted it

bweick · 2021-10-21T21:15:50Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+        override
+        returns (address, uint256, bytes memory)
+    {
+        require(_isValidPool(_pool, _components), "invalid pool");


Same comment as above, this check is duplicated

true, deleted it

bweick · 2021-10-21T21:21:34Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+        address[2] memory expectedTokens = metapoolFactory.get_coins(_pool);
+
+        for (uint256 i = 0; i < _components.length; i++) {
+            if ((_components[i] == expectedTokens[0] || _components[i] == expectedTokens[1]) == false) {


I think we can rewrite this as !(_components[i] == expectedTokens[0] || _components[i] == expectedTokens[1]) == false

I think Brian means:
!(_components[i] == expectedTokens[0] || _components[i] == expectedTokens[1])

bweick · 2021-10-21T21:27:26Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+        address[2] memory expectedTokens = metapoolFactory.get_coins(_pool);
+
+        for (uint256 i = 0; i < _components.length; i++) {
+            if ((_components[i] == expectedTokens[0] || _components[i] == expectedTokens[1]) == false) {


I think we want to check that there are no duplicates in the _components array, right now this would pass if there was a duplicate that was one of the tokens in the AMM.

But since the AmmModule is not yet deployed I say we make that check in the AmmModule itself because I can't think of a scenario where an AMM would have "duplicate" components.

Would add that check here using AddressArrayUtils.hasDuplicate which can be found in contracts/lib

ncitron · 2021-10-21T21:41:18Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+    using SafeCast for int256;
+
+    IMetapoolFactory public metapoolFactory;
+    address public constant THREE_CRV = address(0x6c3F90f043a72FA612cbac8115EE7e52BDe6E490);


Would avoid hardcoding this.You can instead pass it to the constructor. This means it will work with other metapools by just doing a new deployment (such as the btc one).

I think this can be deleted actually because it's not used

yeah its not in use anymore

ncitron · 2021-10-21T21:43:24Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+        address[2] memory expectedTokens = metapoolFactory.get_coins(_pool);
+
+        for (uint256 i = 0; i < _components.length; i++) {
+            if ((_components[i] == expectedTokens[0] || _components[i] == expectedTokens[1]) == false) {


I think Brian means:
!(_components[i] == expectedTokens[0] || _components[i] == expectedTokens[1])

ncitron · 2021-10-21T22:04:06Z

contracts/protocol/integration/amm/CurveFactoryMetapoolAmmAdapter.sol

+    IMetapoolFactory public metapoolFactory;
+    address public constant THREE_CRV = address(0x6c3F90f043a72FA612cbac8115EE7e52BDe6E490);
+
+    string public constant ADD_LIQUIDITY = "add_liquidity(uint256[2],uint256,address)";


You probably want to use the ITriPoolZap functions rather than the IMetaPoolZap ones. This function requires depositing a combination of the 3Pool token and the extra token. The ITriPoolZap interface lets you deposit any of the 3Pool's pools' underlying tokens and the extra token. Makes it a lot more usable.

@ncitron it turns out that this interface & adapter is specifically for the factory metapool and not the legacy metapools. Do you know if the TriPoolZap supports the factory metapools? we had issues with the TriPoolZap reverting without any reason string response, so we ended up switching focus to the factory metapool with the intention to revisit the Zap and legacy metapool implementation afterwards.

I believe they do. I had checked that it works on a whole list of pools. I think one of the gotchas on some of the different pools though is that the LP token address is different than the pool address though. I believe there is a contract for converting from LP token address to pool address and vice versa somewhere in the docs. I had previously confirmed that the zap contract works with the LUSD, alUSD, and USDN pools though (and IIRC alUSD is one of the legacy ones)

updates imports

…otocol-v2 into curve-amm-adapter

cgewecke · 2022-05-18T15:57:20Z

Closed in favor of #253

ncitron and others added 7 commits September 28, 2021 19:28

add CurveMetaPoolAmmAdapter

387074a

wip tests

af36e46

fork test wip

56ab821

curveAmmAdapter #addLiquidity + #removeLiquidity test work

26693ba

#addLiquiditySingleAsset test works

74b6e46

all tests pass

1a07717

renaming adapter, added readme

7fe0ea2

cgewecke reviewed Oct 21, 2021

View reviewed changes

utils/contracts/curve.ts Outdated Show resolved Hide resolved

updates imports

31e4f51

bweick reviewed Oct 21, 2021

View reviewed changes

ncitron reviewed Oct 21, 2021

View reviewed changes

RedVeil and others added 4 commits October 22, 2021 10:08

Merge pull request #1 from anthonymartin/bugfix/fixes-imports-curve-amm

661f544

updates imports

adressed comments

3c45b14

Merge branch 'curve-amm-adapter' of https://github.com/RedVeil/set-pr…

04ac1fd

…otocol-v2 into curve-amm-adapter

Merge branch 'master' into curve-amm-adapter

281164b

cgewecke closed this May 18, 2022

Curve amm adapter #155

Curve amm adapter #155

Uh oh!

Conversation

RedVeil commented Oct 21, 2021

Code Review Processes

New Feature Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonymartin Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgewecke commented May 18, 2022

Uh oh!

Uh oh!

anthonymartin Oct 21, 2021 •

edited

Loading