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

add a failing test for planning add_liquidity on curve.fi #34

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BlinkyStitt
Copy link

@BlinkyStitt BlinkyStitt commented Jan 29, 2022

While trying to port this code to python, I came across what I am pretty sure is a bug planning add_liquidity for 0xbEbc44782C7dB0a1A60Cb6fe97d0b483032FF1C7.

Function signature: add_liquidity(amounts: uint256[3], min_mint_amount: uint256)

It looks like it sees uint256[3] as a dynamic type, so it cuts the first 32 bytes off. Normally that would be the length, but since this type has a known size, that is actual data.

planner.add(
  CurvePool.functions['add_liquidity(uint256[3],uint256)']([1, 2, 3], 4)
);

...

state: [
  '0x00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000003',
  '0x0000000000000000000000000000000000000000000000000000000000000004'
]

I'm not sure what the right fix is.

@BlinkyStitt
Copy link
Author

Right now, when I look at my trace, things are shifted incorrectly:

                    └── Vyper_contract.add_liquidity  [CALL]  14828:20862  [159398294 / 264948254 gas]
                        │   ├── address: 0xbEbc44782C7dB0a1A60Cb6fe97d0b483032FF1C7
                        │   ├── value: 0
                        │   └── input arguments:
                        │       ├── amounts: [64, 191270331176979879371219, 97524790389]
                        │       └── min_mint_amount: 69997057133

Here, amounts should be: [28001643170146867818963, 97524790389, 69997057133]

And min_mint_amount should be: 191270331176979879371219

@decanus
Copy link
Contributor

decanus commented Jan 29, 2022

Wonder if this could relate to: weiroll/weiroll#56 there seem to be several issues with the planner and more "special" types.

@BlinkyStitt
Copy link
Author

Yes, that does look very similar

@BlinkyStitt
Copy link
Author

BlinkyStitt commented Jan 29, 2022

So I've changed my planner (written in Python so I can't update this repo) to treat fixed length arrays specially.

Now, for encoding, it changes a function signature like uint256[3],uint256 into uint256,uint256,uint256,uint256. This seemed easier than changing the solidity contracts.

                    ├── Vyper_contract.add_liquidity  [CALL]  16531:38302  [265426023 / 476902153 gas]
                    │   │   ├── address: 0xbEbc44782C7dB0a1A60Cb6fe97d0b483032FF1C7
                    │   │   ├── value: 0
                    │   │   └── input arguments:
                    │   │       ├── amounts: [28001643170146867818963, 116657060969, 69997057133]
                    │   │       └── min_mint_amount: 209983602241401374147196

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.

2 participants