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

LockstakeClipper Callee #43

Merged
merged 25 commits into from
Aug 14, 2024
Merged

LockstakeClipper Callee #43

merged 25 commits into from
Aug 14, 2024

Conversation

valiafetisov
Copy link
Collaborator

@valiafetisov valiafetisov commented Jul 31, 2024

This PR implements callee contract for the LockstakeClipper using UniswapV2. It allows anyone to participate in auctions using UniswapV2 path to exchange MKR (or NGT) into DAI (or NST).

Notes:

  • CI is expected to fail due to the tests for other callees being fragile / depending on the market
    • To test this specific callee, one can run forge test --match-contract=UniswapV2LockstakeCalleeTest locally
  • In order to write tests for the new callee, we had to update dependencies and remove hardcoded solidity version at the foundry level (--use solc:0.6.12) while strictly setting it in every callee contract instead. This is due to LockstakeEngine requiring at least 0.8.21 solidity version
    • If requested, those changes could be moved into a separate maintenance PR to keep git history clean
  • Current version of the callee support any UniV2 paths that start from MKR or NGT and end with DAI or NST (in other words both MKR/DAI and NGT/NST pools are supported, but not only). The previous version of the callee (found at 9b5cb11) required a redeployment to support different destination token
    • Please let us know which contract design is preferable

@DaiFoundation-DevOps
Copy link

DaiFoundation-DevOps commented Jul 31, 2024

CLA assistant check
All committers have signed the CLA.

@valiafetisov valiafetisov self-assigned this Jul 31, 2024
Copy link

@oldchili oldchili left a comment

Choose a reason for hiding this comment

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

Only looked at the new callee contract, looks good.
Made some minor comments.

src/UniswapV2LockstakeCallee.sol Outdated Show resolved Hide resolved
src/UniswapV2LockstakeCallee.sol Outdated Show resolved Hide resolved
src/UniswapV2LockstakeCallee.sol Outdated Show resolved Hide resolved
src/UniswapV2LockstakeCallee.sol Outdated Show resolved Hide resolved
src/UniswapV2LockstakeCallee.sol Outdated Show resolved Hide resolved
@oldchili
Copy link

oldchili commented Aug 5, 2024

All changes look good, I'm not able to resolve the comments, so please do that on your side.

src/test/UniswapV2LockstakeCallee.t.sol Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Outdated Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Outdated Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Outdated Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Outdated Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Outdated Show resolved Hide resolved
src/test/UniswapV2LockstakeCallee.t.sol Show resolved Hide resolved
Copy link
Contributor

@DaeunYoon DaeunYoon left a comment

Choose a reason for hiding this comment

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

All comments are addressed and the changes look good to me.
Feel free to resolve the above comments as I'm not able to do it.

Copy link

@SidestreamIcedMango SidestreamIcedMango left a comment

Choose a reason for hiding this comment

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

  • Checked callee contract
    • Static Interfaces
      • No unused static interfaces
      • Interface naming style should match with Like suffix (e.g. VatLike)
      • Each static interface declare only functions actually used in the spell code
    • Precision units
      • Precision units match their defined values:
        • WAD = 10 ** 18
        • RAY = 10 ** 27
        • RAD = 10 ** 45
      • Precision units match with Numerical Ranges
    • Ensure every variable is declared as public/internal
  • Checked tests
    • Ensure solc version is matching with contract
    • Checked MockUniswapRouter02
    • Checked UniswapV2LockstakeCalleeTest
      • Function setUp matches with provided existing test function
      • Function _urnSetUp matches with provided existing test function
      • Function _forceLiquidation matches with provided existing test function
      • Checked setUpCallee
        • Correctly sets up test callee
      • Checked _testCalleeTake
        • Simulates and tests multiple actors using the callee to take from an auction
    • Test calls cover all relevant cases
  • Checked formatting
  • Check all tests are passing locally using make test
Ran 16 tests for src/test/UniswapV2LockstakeCallee.t.sol:UniswapV2LockstakeCalleeTest
[PASS] testCalleeTake_NoDelegate_NoStaking_MkrDai() (gas: 3476105)
[PASS] testCalleeTake_NoDelegate_NoStaking_MkrNst() (gas: 3401278)
[PASS] testCalleeTake_NoDelegate_NoStaking_NgtDai() (gas: 3576068)
[PASS] testCalleeTake_NoDelegate_NoStaking_NgtNst() (gas: 3501285)
[PASS] testCalleeTake_NoDelegate_WithStaking_MkrDai() (gas: 3515101)
[PASS] testCalleeTake_NoDelegate_WithStaking_MkrNst() (gas: 3440273)
[PASS] testCalleeTake_NoDelegate_WithStaking_NgtDai() (gas: 3615082)
[PASS] testCalleeTake_NoDelegate_WithStaking_NgtNst() (gas: 3540253)
[PASS] testCalleeTake_WithDelegate_NoStaking_MkrDai() (gas: 3513726)
[PASS] testCalleeTake_WithDelegate_NoStaking_MkrNst() (gas: 3438942)
[PASS] testCalleeTake_WithDelegate_NoStaking_NgtDai() (gas: 3613688)
[PASS] testCalleeTake_WithDelegate_NoStaking_NgtNst() (gas: 3538862)
[PASS] testCalleeTake_WithDelegate_WithStaking_MkrDai() (gas: 3552697)
[PASS] testCalleeTake_WithDelegate_WithStaking_MkrNst() (gas: 3477869)
[PASS] testCalleeTake_WithDelegate_WithStaking_NgtDai() (gas: 3654092)
[PASS] testCalleeTake_WithDelegate_WithStaking_NgtNst() (gas: 3612389)
Suite result: ok. 16 passed; 0 failed; 0 skipped; finished in 50.59s (52.86s CPU time)

@valiafetisov valiafetisov merged commit bc4b4eb into master Aug 14, 2024
1 of 3 checks passed
@valiafetisov valiafetisov deleted the lse-callee branch August 14, 2024 09:15
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.

5 participants