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
feat(spike): taker fee #6034
feat(spike): taker fee #6034
Changes from 76 commits
4eb8594
12dccfa
e857cc9
f773225
0e54b35
4d421fd
8d1c8c6
4177b7a
49e6aff
e57f4a6
547232f
5ea549b
d43a1e5
914cedc
5a68fa6
4312222
f0afc3d
a3863d5
e4294a4
f21dba0
48f1d6f
3fe5742
925d619
f51eb28
aa02dad
bf5af4a
2177b15
3b5453e
e4420d3
0834a30
9a7b1c2
279ae06
323d742
f96efbf
7e2bcf4
71f1e90
1bd9f15
21c7316
89b0fe3
7ec60cc
eeefe36
3ddd408
150eeb1
08bb1ea
a300d6e
411bfa6
236505d
9c17b70
41a1968
c2108b7
d081c2a
4cb0230
5c9fbd8
214dd27
f5ee761
e617f04
7e8f86e
c23c5b5
6caac1f
fbd5700
55be63b
b716e72
0e5d2eb
333dc17
fca43b5
3b4d80c
7563916
72ffbb6
cf5a80c
aab35ca
b2c7d50
cbc2ff4
0f2da85
d129ea3
9a5616d
573565e
07b98f2
bed1140
0ba3702
95bfae1
b01f264
73830bc
518f68b
52e210c
303bfb9
e7b7a20
d5e9123
708442e
759c310
ecaa3f3
87e4310
84c8873
726532e
e48e614
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.
Note to reviewer: Bad diff, only diff is these two lines
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.
Let's get the existing params from state instead of getting defaults.
There is also
PoolCreationFee
. By luck, the default happens to match the mainnet value but might not be the same on testnet.So let's switch to getting the params so that environment-specific PoolCreationFee is preserved.
Please also add the upgrade unit tests to make that params are updated correctly
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.
Note to reviewer: As mentioned earlier, this was moved from CL params to here. It was not removed from CL params yet since we migrate in this upgrade, and then next upgrade we can completely remove from CL.
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.
As of today, we cannot fully delete parameters from a module. There is also a risk of not updating all instances of using these params from CL to poolmanager, and getting invalid state. Given the timeline, I suggest erroring on the side of caution and just importing the CL keeper into the pool manager
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.
In case we do decide to keep this in poolmanager, please make sure:
I still strongly prefer we do not duplicate these even temporarily
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 had massive issues importing CL keeper into pool manager with cyclical imports, but if you can figure it out I'm good with switching to this
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.
OK. Let's not block on this. Could you make an issue to remove the param from CL after the upgrade please?
By remove, I mean marking it as reserved in the proto in the next major upgrade
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.
#6211
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.
Are these percents or ratios that sum to 1?
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.
percents, where the total must sum to 1