sushiswap / trident Goto Github PK
View Code? Open in Web Editor NEWRapid AMM Development Framework
License: GNU General Public License v3.0
Rapid AMM Development Framework
License: GNU General Public License v3.0
There are obvious concerns about the ability to set and execute a migrator on the router, however this gives us a major competitive advantage in combination with the master chef, I think realistically we should include it with the new router to retain this advantage for the future.
Can existing implementation be supported?
Move each pool type into its own folder. Something like trident/contracts/{constantProduct/concentratedLiquidity/hybrid...}
You could probably write a generic pool test where you just feed in the inputs and expected outputs, wdyt @maxsam4
When I try to run trident tests locally, Migration.test.ts
always fails. Tests only pass if I explicitly
git clone https://github.com/sushiswap/trident
cd trident/
yarn install
yarn build
yarn test
...
42 passing (2m)
2 pending
38 failing
1) Migration
"before all" hook for "Should prepare for migration in chef":
ProviderError: Must be authenticated!
at HttpProvider.request (node_modules/hardhat/src/internal/core/providers/http.ts:49:19)
at getNetworkId (node_modules/hardhat/src/internal/hardhat-network/provider/utils/makeForkClient.ts:109:43)
at Object.makeForkClient (node_modules/hardhat/src/internal/hardhat-network/provider/utils/makeForkClient.ts:40:27)
at Function.create (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:153:15)
at HardhatNetworkProvider._init (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:234:46)
at async HardhatNetworkProvider._reset (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:327:5)
at async HardhatModule._resetAction (node_modules/hardhat/src/internal/hardhat-network/provider/modules/hardhat.ts:206:5)
at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:108:18)
at async Context.<anonymous> (test/Migration.test.ts:11:5)
...
The weird thing is it's only only the tests in test/Migration.test.ts
that fail but once they do, every test after them fails with the same message. For example TridentMath
:
TS_NODE_TRANSPILE_ONLY=1 npx hardhat test test/Migration.test.ts test/TridentMath.test.ts
3) TridentMath
"before all" hook for "TridentMath.sqrt() returns correct values":
ProviderError: Must be authenticated!
If your run tests for TridentMath
only, they succeed:
TS_NODE_TRANSPILE_ONLY=1 npx hardhat test test/TridentMath.test.ts
TridentMath
✓ TridentMath.sqrt() returns correct values (196ms)
1 passing (2s)
Get this error
TypeError: hardhat_1.ethers.getNamedSigner is not a function
After run
npm run test
on master branch
There are two types of Trade which guarentee determisim in their own respects, exact input or exact output.
In my opinion a genius design decision in earlier versions of AMMs, however Trident only supports a deterministic input which is problematic for a number of reasons and common use cases of an AMM.
ExactIn - You have a specific input, the input is determistic, you will never use more input than you expect to recieve some margin (slippage) of output.
This is better understood in my mind as a seller looking to sell a specific amount of tokens, with margin on the output.
ExactOut - You have a specific output, the output is determistic, you will never get more output than you expect in return for some margin (slippage) of input.
This is better understood in my mind as a buyer looking to buy a specific amount of tokens, with margin on the input.
These are two distinct, or rather inverse functions, which have their own use cases. I believe the decision was originally made to include them in earlier versions of AMMs for a good reason, and belive we need to support or else we we lose the ability to cater for the latter case, ExactOut.
Examples:
You could list a substantial number of common cases like this which require exact out to be both determistic and reliable.
Solutions:
And functionality added to support exactOut for each pool type.
Hacky solutions:
In both cases above this will require a substantially rethink and evaluation of the UI from top to bottom, supporting libraries, and other areas of the application outside of trident too. Timescale for this would be an unknown and substantial, and I believe results would not be great in terms of UX and predictability.
Problem: The UI needs to find the relevant pool addresses for two tokens.
Solution: When deploying an index pool store the pool address under a pools mapping so that every token combination is covered (n*(n-1)/2) combinations.
for(let i = 0; i < pools.length; i++)
for(let j = i + 1; j < pools.length; j++)
pools[tokens[i]][tokens[j]] = pool
This is more of a nit pick than anything else, but events should always be named in a way which represents an event, meaing that they should be in past tense.
DeployPool -> DeployedPool/PoolDeployed
TransferOwner -> TransferedOwnership/OwnershipTransfered
TransferOwnerClaim -> ClaimedOwnership/OwnershipClaimed
There seems to be an implicit dependency between test suites because whether they fail or not depends on their order. In particular _ConstantProduct.test.ts
fails if you run Router.test.ts
before it.
This is especially problematic because to work around #283 I need to exclude some tests. Hardhat does not seem to have a command-line option for that so I'm doing it like this:
tests_to_run=$(find test/ -name "*.test.ts" ! -path "test/migration/Migration.test.ts")
npx hardhat test $tests_to_run
This unfortunately may produce a different order than the one Hardhat uses.
_ConstantProduct.test.ts
and Router.test.ts
This fails:
npx hardhat test test/Router.test.ts test/constant-product/_ConstantProduct.test.ts
Constant Product Pool Old
1) "before all" hook in "Constant Product Pool Old"
9 passing (6s)
1 failing
1) Constant Product Pool Old
"before all" hook in "Constant Product Pool Old":
AssertionError: expected '0x4B8297ba8ab03Ad9Dc81d09edD9EAB58e7d1100a' to equal '0xAD806Ef1a19DB65F12C08E0f6101Eb4cD5004CCd'
+ expected - actual
-0x4B8297ba8ab03Ad9Dc81d09edD9EAB58e7d1100a
+0xAD806Ef1a19DB65F12C08E0f6101Eb4cD5004CCd
at initialize (test/harness/ConstantProduct.ts:99:25)
at async Context.<anonymous> (test/constant-product/_ConstantProduct.test.ts:8:5)
Reversing the order fixes the problem:
npx hardhat test test/constant-product/_ConstantProduct.test.ts test/Router.test.ts
Constant Product Pool Old
#swap
✓ Should do 24 types of swaps (3300ms)
#mint
✓ Balanced liquidity to a balanced pool (109ms)
✓ Add liquidity in 16 different ways before swap fees (1938ms)
✓ Add liquidity in 16 different ways after swap fees (1973ms)
#burn
✓ Remove liquidity in 12 different ways (2649ms)
...
14 passing (18s)
If you sort the paths to work around it, it still fails, the errors are just different. The test suite seems to be very brittle with regard to test ordering.
tests_to_run=$(find test/ -name "*.test.ts" ! -path "test/migration/Migration.test.ts" | LC_ALL=C sort)
npx hardhat test $tests_to_run
113 passing (3m)
3 pending
2 failing
1) Constant Product Pool Old
#mint
Add liquidity in 16 different ways before swap fees:
AssertionError: Expected "49000000000000000000" to be equal 0
+ expected - actual
{
- "_hex": "0x00"
+ "_hex": "0x02a802f8630a240000"
"_isBigNumber": true
}
at assertArgsArraysEqual (node_modules/@ethereum-waffle/chai/dist/cjs/matchers/emit.js:58:54)
at tryAssertArgsArraysEqual (node_modules/@ethereum-waffle/chai/dist/cjs/matchers/emit.js:65:20)
at /tmp/ext-test-Trident-fjO08G/ext/node_modules/@ethereum-waffle/chai/dist/cjs/matchers/emit.js:77:13
at runMicrotasks (<anonymous>)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at runNextTicks (node:internal/process/task_queues:65:3)
at listOnTimeout (node:internal/timers:536:9)
at processTimers (node:internal/timers:510:7)
at async addLiquidity (test/harness/ConstantProduct.ts:149:3)
at async addLiquidityInMultipleWays (test/constant-product/_ConstantProduct.test.ts:73:7)
2) Constant Product Pool Old
#mint
Add liquidity in 16 different ways after swap fees:
AssertionError: Expected "35000000000000000000" to be equal 0
+ expected - actual
{
- "_hex": "0x00"
+ "_hex": "0x01e5b8fa8fe2ac0000"
"_isBigNumber": true
}
at assertArgsArraysEqual (node_modules/@ethereum-waffle/chai/dist/cjs/matchers/emit.js:58:54)
at tryAssertArgsArraysEqual (node_modules/@ethereum-waffle/chai/dist/cjs/matchers/emit.js:65:20)
at /tmp/ext-test-Trident-fjO08G/ext/node_modules/@ethereum-waffle/chai/dist/cjs/matchers/emit.js:77:13
at runMicrotasks (<anonymous>)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at runNextTicks (node:internal/process/task_queues:65:3)
at listOnTimeout (node:internal/timers:536:9)
at processTimers (node:internal/timers:510:7)
at async addLiquidity (test/harness/ConstantProduct.ts:149:3)
at async addLiquidityInMultipleWays (test/constant-product/_ConstantProduct.test.ts:73:7)
Thank you for the awesome Sushi team for shipping an excellent product.
As the product is quite complex, does there exist visual documentation regarding the Trident contract set how the different contracts are related and interact? For example, a UML diagram would help external developers to get up to the speed up fast.
Isolated test here: https://github.com/sushiswap/trident/blob/master/test/IndexPool.test.ts
IndexPool is currently failing on swap. The _getAmountOut function is returning an unexpectedly large number that causes an overflow when outRecord.amount is updated.
Also appears to be a logic error in the y value calculation - dividing tokenbalanceIn by itself
function _getAmountOut(
uint256 tokenBalanceIn,
uint256 tokenWeightIn,
uint256 tokenBalanceOut,
uint256 tokenWeightOut,
uint256 tokenAmountIn
) internal view returns (uint256 finalAmountOut) {
uint256 weightRatio = tokenWeightIn / tokenWeightOut;
uint256 adjustedIn = BASE - swapFee;
adjustedIn = tokenAmountIn * adjustedIn;
uint256 y = tokenBalanceIn / tokenBalanceIn + adjustedIn; // tokenBalanceIn/tokenBalancein ?
uint256 foo = pow(y, weightRatio);
uint256 bar = BASE - foo;
finalAmountOut = tokenBalanceOut * bar;
}
trident/contracts/pool/concentrated/ConcentratedLiquidityPool.sol
Lines 561 to 566 in ef1f4ed
When calculating feeGrowth, currentTick must be calculated based on the current price for it to be calculated correctly.
int24 currentTick = nearestTick;
int24 currentTick = TickMath.getTickAtSqrtRatio(price);
referenced : ConcentratedLiquidityPoolManager.sol
Is there any reason why it is possible to call only msg.sender
?
require(msg.sender == ownerOf(tokenId), "NOT_ID_OWNER");
If this happens, it will be difficult to create a 3rd party contract for the position NFT in the future. How about changing it to _isApprovedOrOwner
as shown below?
require(_isApprovedOrOwner(msg.sender, tokenId), "NOT_ALLOWED");;
trident/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol
Lines 128 to 136 in 8b183b1
trident/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol
Lines 167 to 172 in 8b183b1
Pool implementations require a poolIdentifier to satisfiy IPool.
/// @return A unique identifier for the pool type.
function poolIdentifier() external pure returns (bytes32);
However, uniqueness is not enforced, it probably makes sense to do this.
I did suggest an alternative too which might be worth thinking about, and could be quite useful for consumers, a pool indentifier + version e.g ID: ConstantProductPool, VERSION: 1 etc..., uniqueness of the combination would need to be enforced.
Using CREATE2 for pool deployment has various benefits we should seriously consider. WETH10 AFAIK will be deployed using CREATE2, V2 pools are deployed using CREATE2, and I think the next gen AMM should probably follow suit with the initialiser pattern.
https://github.com/sushiswap/sushiswap/blob/canary/contracts/uniswapv2/UniswapV2Factory.sol#L38
https://blog.openzeppelin.com/getting-the-most-out-of-create2/
If you try to build the project and run tests without relying on the exact versions listed in yarn.lock
, many tests fail. Some of the dependencies listed in package.json
must be incorrect or not restricted enough.
test:coverage
git clone https://github.com/sushiswap/trident
cd trident/
rm yarn.lock
yarn install
yarn build
yarn test:coverage
> Istanbul reports written to ./coverage/ and ./coverage.json
Error in plugin solidity-coverage: TSError: ⨯ Unable to compile TypeScript:
test/router/helpers/index.ts:1:30 - error TS2305: Module '"@sushiswap/tines"' has no exported member 'findMultiRoute'.
1 import { RToken, MultiRoute, findMultiRoute } from "@sushiswap/tines";
~~~~~~~~~~~~~~
test
TS_NODE_TRANSPILE_ONLY=1 \
npx hardhat test $(
find test/ -name "*.test.ts" ! -path "test/Migration.test.ts"
)
...
27 passing (31s)
2 pending
43 failing
1) MultiPool Routing Tests - Random Topologies & Random Swaps
Random topology output prediction precision is ok:
TypeError: (0 , tines_1.findMultiRoute) is not a function
at Object.createRoute (test/router/helpers/index.ts:40:31)
at Context.<anonymous> (test/router/RoutingMultiPoolComplex.test.ts:51:34)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at runNextTicks (internal/process/task_queues.js:66:3)
at listOnTimeout (internal/timers.js:523:9)
at processTimers (internal/timers.js:497:7)
...
9) HybridPool Typescript == Solidity check
Check regular liquidity values
Test 1:
swap amount not close enough to predicted amount
+ expected - actual
-false
+true
at checkSwap (test/router/RoutingHybrid.test.ts:162:72)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at runNextTicks (internal/process/task_queues.js:66:3)
at listOnTimeout (internal/timers.js:523:9)
at processTimers (internal/timers.js:497:7)
at async Context.<anonymous> (test/router/RoutingHybrid.test.ts:188:11)
...
40) Concentrated Pool Routing
swap without crossing:
AssertionError: expected NaN to be below 1e-12
+ expected - actual
-NaN
+1e-12
at Context.<anonymous> (test/router/RoutingCL.test.ts:71:54)
at runMicrotasks (<anonymous>)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at runNextTicks (internal/process/task_queues.js:66:3)
at listOnTimeout (internal/timers.js:523:9)
at processTimers (internal/timers.js:497:7)
This should theoretically be cheaper since low-level calls do not check for extcode. However, gas should be compared before and after on the target Solidity version.
typo/logic err in the _getAmountOut
function of IndexPool.sol
see: L269 where it is dividing tokenBalanceIn
by itself.
the _getAmountOut function in IndexPool.sol
is currently throwing an under/overflow error. see tests on this branch: RoutingIndex.test.ts.
https://github.com/sushiswap/trident/blob/master/contracts/pool/hybrid/HybridPool.sol#L441
getAmountIn is unimplemented in Hybrid Pool immediately reverting, would you like me to implement it and submit a PR (not sure on contributing guidelines sorry!)
One thing I've run into immediately with the subgraph is unable to distinguish with the NewPoolCreated event what factory this is related to since it contains a pool address only and no context on the factory which created it, nor any data to build this pool without making external calls which should be avoided in the subgraph.
I think we may need to include separate events for each pool type since they can have different initial parameters, like constant product (token0, token1, swapFee), hybrid (token0, token1, swapFee, a), etc... but I'm not entirely sure the best way to proceed here, I would be interested on your thoughts.
Other potentially missing events
Gathering data via Web3 is not viable via public metered RPCs across networks at the moment
Suggestions
Howdy,
The link to ConcentratedLiquidityPool via trident/blob/master/contracts/pool/concentrated/ConcentratedLiquidityPool.sol
doesn't seem to lead to a valid file.
Is this code forthcoming?
when the value of lower
is TickMath.MIN_TICK
and upper
is TickMath.MAX_TICK
, I think it should be passed _ensureTickSpacing()
method. But current _ensureTickSpacing()
block this case.
My Suggestion is below.
trident/contracts/pool/concentrated/ConcentratedLiquidityPool.sol
Lines 440 to 445 in ef1f4ed
function _ensureTickSpacing(int24 lower, int24 upper) internal view {
if (lower != TickMath.MIN_TICK) {
if (lower % int24(tickSpacing) != 0) revert InvalidTick();
if ((lower / int24(tickSpacing)) % 2 != 0) revert LowerEven();
}
if (upper != TickMath.MAX_TICK) {
if (upper % int24(tickSpacing) != 0) revert InvalidTick();
if ((upper / int24(tickSpacing)) % 2 == 0) revert UpperOdd();
}
}
IndexPool mint function has strange behaviour:
test:
tests\IndexPool.test.ts'pool balance should be equal to transferred value'
We should be aiming for complete code coverage, at least of relevent contracts for intial release.
Master Deployer is already covered.
ConstantProductPoolFactory, ConstrantProductPool & Router need some work to hit 100%.
ConcentratedLiquidity pools need additional checks to validate balances
In the existing tests that add liquidity to a CL pool should have and additional requirement to remove all liquidity and then validate the pool balances.
There is an expectation of remaining dust in the pool. The acceptable value is less than a value less that 10 * (10 ** 18) of any given token.
../tasks/add-liquidity.ts:2:80 - error TS2307: Cannot find module '../types' or its corresponding type declarations.
2 import type { BentoBoxV1, BentoBoxV1__factory, ERC20Mock, TridentRouter } from "../types"
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.