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

feat:PoolManager Interface Implementation v3 #102

Closed
wants to merge 5 commits into from

Conversation

LCJove
Copy link
Collaborator

@LCJove LCJove commented Jul 23, 2024

基于 pr #97 做了部分修改:

  1. 保存的是 factory address 不是 Factory 实例(review 一下是否合理)
  2. 函数内使用 memory 而不是 storage

Copy link

vercel bot commented Jul 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wtf-dapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 23, 2024 6:01am

@@ -1,35 +1,115 @@
// SPDX-License-Identifier: GPL-2.0-or-later
// Author: Eli Lee
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Author: Eli Lee

代码里面就不体现 Author 了。


function getPoolInfo(
address token0,
address token1,
uint24 fee
) external view override returns (PoolInfo memory poolInfo) {}
) external view override returns (PoolInfo memory poolInfo) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

getPoolInfo 第三个参数 fee 应该要改为 index,后面调整过设计,每个代币对下面可以有多个 pool,通过 index 来索引,每个 pool 的 fee 和价格上下限都可以不一样

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

后面根据新的接口定义改一下

Copy link
Collaborator

Choose a reason for hiding this comment

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

#105 可以先看看这个 PR,先修复了初始化代码中之前漏掉的几个问题

Copy link
Contributor

Choose a reason for hiding this comment

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

getPoolInfo 第三个参数 fee 应该要改为 index,后面调整过设计,每个代币对下面可以有多个 pool,通过 index 来索引,每个 pool 的 fee 和价格上下限都可以不一样

我觉得这里想通过代码索引到池子得话这个可以通过初始化池子得方式将代币和池子关联索引。这样是否更合理一点

Copy link
Collaborator

Choose a reason for hiding this comment

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

#107 可以看一下这个,我重新梳理了一下,PoolManager 直接继承 Factory,接口重新设计了一下。


contract PoolManager is IPoolManager {
address public factory;
Copy link
Collaborator

Choose a reason for hiding this comment

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

用 address 应该是没问题的,我看了 v2 https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router01.sol#L12C39-L12C46 和 v3 https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/PeripheryImmutableState.sol#L10 的类似的场景,都是存的 address。

不过还加了 immutable,我们也可以加上,应该是可以有一些优化节约点 gas。

Copy link
Collaborator

Choose a reason for hiding this comment

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

另外保存地址和实例看上去差不多:
image

我们这个场景我感觉保存地址应该理论上更节约 gas。getPools 这些都是只读的方法,不需要消耗 gas。所以把实例化放到那里面应该是没问题的。

Copy link
Contributor

Choose a reason for hiding this comment

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

直接存地址的方式会更节省gas但是差别不是很大,存实例的话会更加可读一点。存入实例也会多做一步静态检查。两种方式都可以。


function getPoolInfo(
address token0,
address token1,
uint24 fee
) external view override returns (PoolInfo memory poolInfo) {}
) external view override returns (PoolInfo memory poolInfo) {
address poolAddress = IFactory(factory).getPool(token0, token1, fee);
Copy link
Collaborator

Choose a reason for hiding this comment

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

最新的代码 getPool 的第三个参数是 index 了。

P102_InitContracts/code/PoolManager.sol Show resolved Hide resolved
}

// 创建 pool
pool = IFactory(factory).createPool(params.token0, params.token1, params.fee);
Copy link
Collaborator

Choose a reason for hiding this comment

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

同上,fee 应该要改为 index

Copy link
Contributor

Choose a reason for hiding this comment

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

这个有个问题IPoolManager.sol接口中的入参的结构体CreateAndInitializeParams是不是也需要改。
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

fee 应该还是需要的,每个 Pool 都有自己的 fee,只是看是否需要加 index。

现在的逻辑是这样:每个代币对下面会有多个 Pool,每个 Pool 都有各自的 fee 和 tickLower 和 tickUpper,在 PoolManager 里面同一个代币对下面的 Pool 会放到一个数组中,所以也可以通过 index 索引到。

Copy link
Contributor

Choose a reason for hiding this comment

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

初始化池子时候获取池子方法(getPool)也需要传入index去获得这里的token0、tokne1、index如果通过这三个参数作为参数获取池子判断池子是否存在会变得麻烦,没有通过token0、tokne1、fee 能保证池子会有唯一性。

Copy link
Collaborator

Choose a reason for hiding this comment

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

token0、tokne1、fee、tickLower、tickUpper,这五个才能保证池子唯一

Copy link
Collaborator

Choose a reason for hiding this comment

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

嗯,缺少,这个要看看怎么调整,你有什么想法吗?

Copy link
Collaborator

Choose a reason for hiding this comment

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

看了一下,应该用 getPools 来拿到一个代币对下面所有的池子来判断

Copy link
Collaborator

Choose a reason for hiding this comment

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

Factory 的接口应该是有点问题的,这个 createPool 和 getPool 参数应该都要改为 token0、token1、fee、tickLower、tickUpper,这个五个值确认一个池子

Copy link
Contributor

Choose a reason for hiding this comment

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

我觉得池子的唯一性用token0、tokne1、fee这三个参数就可以决定。 tickLower、tickUpper只是定义了可提供流动性的头寸不应该作为池子唯一性的判断条件。

Copy link
Collaborator

Choose a reason for hiding this comment

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

我们现在的设计是把每个池子都单独定义了 tickLower、tickUpper,这个两个和 fee 一样是池子的属性,和 V3 的逻辑不太一样。可以参考 https://github.com/WTFAcademy/WTF-Dapp/blob/main/P003_OverallDesign/readme.md 这里的说明

IPool(pool).initialize(params.sqrtPriceX96, params.tickLower, params.tickUpper);

// 添加 PoolKey 到 poolKeys 数组
PoolKey memory poolKey = PoolKey({
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 memory 是必须的吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

结构体必须指定类型不然编译器会报错:
TypeError: Data location must be "storage", "memory" or "calldata" for variable, but none was given.
--> PoolManager.sol:94:9:
|
94 | PoolKey key = PoolKey({
| ^^^^^^^^^^^

});
poolKeys.push(poolKey);

// 检查并添加代币到 tokens 数组
Copy link
Collaborator

Choose a reason for hiding this comment

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

有没有必要搞一个 tokenExists 来装满检测 token 是否存在?可以探讨一下

Copy link
Contributor

Choose a reason for hiding this comment

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

加上一个映射应该会提高查询效率,但是会浪费一些存储空间。感觉加上会更好一些?

Copy link
Collaborator

Choose a reason for hiding this comment

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

嗯,这个还好。其实最好的办法是这些信息应该是存服务端就好了,Uniswap 其实就是这么做的,这个合约其实不是必须的。写具体课程内容的时候要提醒一下,避免误导开发者。非必要的内容完全 Web2 的中心化服务器存储就行。

@yutingzhao1991
Copy link
Collaborator

最新的代码在 #114

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.

3 participants