Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 LBPool #1001
Add LBPool #1001
Changes from 47 commits
c03edfb
ca249b3
711609f
b7c4050
1512487
410a9c4
1aa5e65
4c1253d
e3fb44a
3433090
174aa67
eee1f28
b9bbbaf
652e4b5
92dda53
40c5577
9146e8b
336f899
0bdb84b
3a8b009
37cc15c
1887169
2973ad1
9348242
5a2be27
3bedcd8
73275e0
366aefc
9c06b00
78574f1
605d5e8
fca2e63
b65cf10
f1b3b42
aec77e9
af0dcf2
73f27d4
87ea6ff
26fc3ed
b9e1d7a
d93f265
a686bba
a8c8ac7
3f82b06
78780d5
c5fc58e
1e5dd53
e979db7
c72c153
eb7e4f4
f9845ad
1bc1061
29a2ccc
f0da637
4ddeaa0
9405b03
99741e7
50a6852
bf4a35b
a338167
93246a9
8a86a64
2f0acaf
0df5967
c604ba2
9b64acb
5de8945
46719fc
be1c8e8
dfd563c
4c29466
b870b77
8ef0572
429c75a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason in principle for this to change, but it does pose a maintainability question in case we ever migrate routers and the SDK / frontend need to adapt.
If we expect the lifecycle of these pools to be rather bounded I guess this is fine, as it's the simplest / cheapest approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's add a getter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other reason is Daniel said we did want to support at least two from launch: basic and batch. If we have the registry (see PR #1296), we could use that. It would be equivalent to gerrg's original "provider" idea, and the maintenance would be done in the registry, so we wouldn't need to worry about it here. (And then we don't need a getter.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would look like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like an overkill really.
The trusted router is only used to add liquidity, and block non-owners from adding liquidity. You can still use any router to swap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... so really the only benefit would be not having to redeploy if we update the basic router (which should be rare compared to adding a new router or updating something more relay-like such as the CLR router)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jubeira good point that the router only affects liquidity provision. Definitely makes it less of an issue, though I do still think it would be best to allow any "approved" router
For more context, the singular trusted router setup that is in the contract was 100% meant to be a placeholder until there was a router provider/contact registry. Simplifying it to be a single router made it possible for me to get unblocked in the meantime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I'll double check.
I think this is a good starting point anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about initialize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well initialize doesn't have the router argument, so we can't do the same approach.
But we can create and initialize in the same tx. I'll add a method in the factory.