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
Implement Curve Pool Booster. #2327
base: master
Are you sure you want to change the base?
Implement Curve Pool Booster. #2327
Changes from 10 commits
510755d
ca8e2ff
d391fce
b74ed90
e65f962
b3de3b0
3ede65d
fe87911
3adc712
b3ed12d
330ca5d
de03aa9
a4c63d8
22b9e69
03eb2d6
8aeab0e
566f543
85ebeb1
9d90521
38d6dd6
2c31f26
fc82bcb
94ee9f8
345438e
0aed8a9
18dc920
e8445ce
7a32e47
05ca2c0
f2dcaec
4de93ff
e98d5a2
4ec38c8
2b8e459
345cbb9
efc34e7
8f14224
aeeac7c
4cb5e60
3bc438c
f0b9cb4
c75f256
523b927
56e36aa
1a80b6d
c4c2c1e
49a88dd
f9d4d9b
8d33a72
82770e6
b7baf9d
6627d35
5d19df6
92c2daa
7baaa08
4ddbbb2
fa1af7f
7a85a76
4ca8b45
bfab1e2
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.
Is this the manager address on mainnet or on L2? I tried looking at the docs and couldn't find anything relevant
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 last time I discussed with the StakeDAO team was on the mainnet and L2. Let me ask again.
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.
Confirmed by StakeDAO team, both mainnet and L2 👍
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.
So, we need the contract at same address on both chains?
Edit: I know we won't be deploying it on L2 but will we ever need that access to that address there?
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.
From what I understand, no. The only reason we might need it could be to close a campaign, so 24 weeks after the campaign ends. However, as this implies token bridging and so on, that is something that's not available on the
ICampaignRemoteManager
.As it should not happen really often, we can ask the StakeDAO team to close it for us.
Check warning on line 167 in contracts/contracts/strategies/CurvePoolBooster.sol
Codecov / codecov/patch
contracts/contracts/strategies/CurvePoolBooster.sol#L167
Check warning on line 171 in contracts/contracts/strategies/CurvePoolBooster.sol
Codecov / codecov/patch
contracts/contracts/strategies/CurvePoolBooster.sol#L171
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.
Why is this necessary? to recover any accidentally sent tokens? If yes, can you change it to
onlyGovernor
? Also, we typically have atransferTokens
method for ERC20 tokens as wellThere 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.
Not really. This contract will hold a bit of ETH, as we need to pay a bit when calling the CampaignRemoteManager (CCIP fees). So it is in the situation where the pool booster will not be used anymore (end of services) and the operator what to claim the remaining ETH.
And using a payable function is not the best (imo) because a surplus of ETH is sent back to this contract, not to the sender.
What do you think of keeping
onlyOperator
modifier then?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.
IMO it is fine, as there will only be small amount of ETH on the contract whose purpose is to fuel the gas for transactions and that is not part of the user funds.
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.
Yes, exactly, only to fuel gas transactions, nothing more.