feat(contracts): Add LP logic #6

chrismaree · 2022-01-24T20:13:44Z

This PR adds liquidity addition/removal logic.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2022-01-25T02:13:35Z

test/HubPool.LiquidityProvision.ts

+    // Next, remove half the liquidity as ETH (set sendETH = true). This should modify the eth bal but not the weth bal.
+    await expect(() =>
+      hubPool.connect(liquidityProvider).removeLiquidity(weth.address, amountToLp.div(2), true)
+    ).to.changeEtherBalance(liquidityProvider, amountToLp.div(2)); // There should be ETH transferred, not WETH.


this syntax is great! unfortunately it cant be chained along with a .to.changeTokenBalance so I did this the old fashion way.

chrismaree · 2022-01-25T02:14:06Z

test/HubPool.TokenWhitelisting.ts

@@ -0,0 +1,38 @@
+import { expect } from "chai";


notice that I've separated sets of tests into files. goal is to not have one massive file and rather just run some fixtures and have fewer shorter test files.

nicholaspai · 2022-01-25T12:55:43Z

hardhat.config.ts


 const config: HardhatUserConfig = {
-  solidity: "0.8.11",
+  solidity: { compilers: [{ version: "0.8.11", settings: { optimizer: { enabled: true, runs: 200 } } }] },


Set runs to 1_000_000?

nicholaspai · 2022-01-25T12:56:05Z

test/HubPool.Fixture.ts

+import { ethers } from "hardhat";
+import { Contract, BigNumber } from "ethers";
+
+export async function deployHubPoolTestHelperContracts(deployerWallet: any) {


nicholaspai

Awesome first PR on the HubPool lgtm!

mrice32

Looks great! A few minor comments and high level questions

mrice32 · 2022-01-25T16:01:04Z

test/HubPool.TokenWhitelisting.ts

+import { getContractFactory } from "./utils";
+import { deployHubPoolTestHelperContracts } from "./HubPool.Fixture";
+
+let hubPool: Contract, weth: Contract, owner: any, liquidityProvider: any, other: any;


Don't signers have a type we can use?

I could not easily find. will try again

mrice32 · 2022-01-25T16:01:47Z

test/HubPool.LiquidityProvision.ts

-let timer: Contract;
+let hubPool: Contract, weth: Contract, usdc: Contract, dai: Contract;
+let wethLpToken: Contract, usdcLpToken: Contract, daiLpToken: Contract;
+let owner: any, liquidityProvider: any, other: any;


Same here. I think there's a simple type we could use for these

mrice32 · 2022-01-25T16:02:18Z

test/HubPool.Fixture.ts

+}
+
+export async function seedWallet(
+  walletToFund: any,


mrice32 · 2022-01-25T16:02:35Z

hardhat.config.ts


 const config: HardhatUserConfig = {
-  solidity: "0.8.11",
+  solidity: { compilers: [{ version: "0.8.11", settings: { optimizer: { enabled: true, runs: 200 } } }] },


mrice32 · 2022-01-25T16:03:15Z

hardhat.config.ts

      url: process.env.ROPSTEN_URL || "",
-      accounts:
-        process.env.PRIVATE_KEY !== undefined ? [process.env.PRIVATE_KEY] : [],
+      accounts: process.env.PRIVATE_KEY !== undefined ? [process.env.PRIVATE_KEY] : [],


Why not use a mnemonic? Isn't that the preferred way of generating accounts?

I did not change this, this was just a listing change. I think we should ignore this until we get there but ye I agree a mnemonic would be better

mrice32 · 2022-01-25T16:04:22Z

contracts/HubPool.sol

     *************************************************/

-    function addLiquidity(address token, uint256 amount) public {}
+    function addLiquidity(address l1Token, uint256 l1TokenAmount) public payable {


Is this all new code? Or is it all copied? Or is it a mix? If a mix, can you comment on the parts that you have changed (throughout the file)?

it's copied but changed to include the new mapping structure. the v1 version of this did not specify the l1Token (one token per pool). the changes therefore are simply to define the l1Token within the mapping. the other small change is the mint logic is now called on the external contract, rather than on this as the LPtoken is external.

mrice32 · 2022-01-25T16:13:29Z

contracts/HubPool.sol

+        ExpandedERC20 lpToken = new ExpandedERC20(
+            append("Across ", IERC20Metadata(l1Token).name(), " LP Token"), // LP Token Name
+            append("Av2-", IERC20Metadata(l1Token).symbol(), "-LP"), // LP Token Symbol
+            IERC20Metadata(l1Token).decimals() // LP Token Decimals
+        );


Minor note: we could use a contract like the TokenFactory here down the line so the ERC20 bytecode doesn't need to live in this contract (and thus, we give ourselves more room in the bytecode limit).

this is a good point. we can make this change if we run out of space. I'll add a comment.

mrice32 · 2022-01-25T16:16:49Z

contracts/HubPool.sol

 import "@uma/core/contracts/common/implementation/Testable.sol";
 import "@uma/core/contracts/common/implementation/Lockable.sol";
 import "@uma/core/contracts/common/implementation/MultiCaller.sol";
+import "@uma/core/contracts/common/implementation/ExpandedERC20.sol";


nit: something we might consider for a follow-up. We could swap this out for an ERC20 contract that's optimized for this use case (immutable and singular minter/burner).

good point. will add a comment.

mrice32 · 2022-01-25T16:21:25Z

contracts/HubPool.sol

    mapping(address => LPToken) public lpTokens; // Mapping of L1TokenAddress to the associated LPToken.

-    constructor(address timerAddress) Testable(timerAddress) {}
+    event LiquidityAdded(address l1Token, uint256 amount, uint256 lpTokensMinted, address indexed liquidityProvider);


Should we index the l1Token?

sure & done.

mrice32 · 2022-01-25T16:21:39Z

contracts/HubPool.sol


-    constructor(address timerAddress) Testable(timerAddress) {}
+    event LiquidityAdded(address l1Token, uint256 amount, uint256 lpTokensMinted, address indexed liquidityProvider);
+    event LiquidityRemoved(uint256 amount, uint256 lpTokensBurnt, address indexed liquidityProvider);


Should this include the l1Token?

good catch, it should. done and indexed

mrice32 · 2022-01-25T16:23:12Z

contracts/HubPool.sol

+    }
+
+    // Added to enable the BridgePool to receive ETH. used when unwrapping Weth.
+    receive() external payable {}


Should we revert if this comes from an address that is not the WETH contract?

that's an interesting idea but its somewhat inconsistant. we dont prevent users from sending tokens so why should we prevent them from sending eth?

I see what you're saying. ETH transfers work differently, so I would argue it's more likely to send ETH accidentally. However, this would incur a slight gas cost, so it probably isn't worth the cost of the check.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Co-authored-by: Dong-Ha Kim <dongha.kim210@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

* chore: formatting fix (#1) Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * Arbitrum, Ink, Mode, ZkSync * chore: Deploy Blast SpokePool implementation (#3) * chore: Deploy AlephZero SpokePool implementation (#6) * chore: Deploy WorldChain SpokePool implementation (#5) * deploy base lisk eth redstone scroll soneium spoke implementations (#4) Signed-off-by: Matt Rice <matthewcrice32@gmail.com> Signed-off-by: bennett <bennett@umaproject.org> Co-authored-by: Matt Rice <matthewcrice32@gmail.com> * chore: add deployments for linea, optimism, polygon, and zora (#7) Signed-off-by: Matt Rice <matthewcrice32@gmail.com> * fix(test): Require transfer to msg.sender on fill (#8) * fix(artifacts): update artifacts with implementation address Signed-off-by: bennett <bennett@umaproject.org> --------- Signed-off-by: Matt Rice <matthewcrice32@gmail.com> Signed-off-by: bennett <bennett@umaproject.org> Co-authored-by: nicholaspai <npai.nyc@gmail.com> Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com> Co-authored-by: bmzig <57361391+bmzig@users.noreply.github.com> Co-authored-by: bennett <bennett@umaproject.org>

nit

42179cb

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nicholaspai approved these changes Jan 24, 2022

View reviewed changes

chrismaree added 5 commits January 24, 2022 14:03

nit

1e37ad4

Signed-off-by: chrismaree <christopher.maree@gmail.com>

wip

a0189b6

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Delete settings.json

f2871b3

Added liquidity provision methods

d6e771f

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

f28bb71

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree commented Jan 25, 2022

View reviewed changes

nicholaspai reviewed Jan 25, 2022

View reviewed changes

nicholaspai approved these changes Jan 25, 2022

View reviewed changes

mrice32 reviewed Jan 25, 2022

View reviewed changes

UPDATE

5e3facf

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree merged commit 7327ad8 into master Jan 25, 2022

chrismaree deleted the chrismaree/add-lp-logic-2 branch January 25, 2022 17:24

pxrl pushed a commit that referenced this pull request Feb 2, 2024

feat(polygon-zk-evm): add adapter and spokepool (#6)

199dbe7

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai pushed a commit that referenced this pull request Feb 2, 2024

feat(polygon-zk-evm): add adapter and spokepool (#6) (#415)

bf08c4d

Co-authored-by: Dong-Ha Kim <dongha.kim210@gmail.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai pushed a commit that referenced this pull request Feb 5, 2024

feat(polygon-zk-evm): add adapter and spokepool (#6)

92cd39b

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 pushed a commit that referenced this pull request Feb 3, 2025

chore: Deploy AlephZero SpokePool implementation (#6)

da9444a

feat(contracts): Add LP logic #6

feat(contracts): Add LP logic #6

Uh oh!

Conversation

chrismaree commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

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!

mrice32 Jan 25, 2022 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrismaree commented Jan 24, 2022 •

edited

Loading

mrice32 Jan 25, 2022 •

edited

Loading