Add closeCampaign() to CurvePoolBooster.
#2360
Conversation
clement-ux
changed the title
closeCampaign() to CurvePoolBooster.
clement-ux
requested review from
sparrowDom,
DanielVF,
naddison36 and
shahthepro
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## clement/pool-booster-curve #2360 +/- ##
==============================================================
- Coverage 51.98% 51.46% -0.52%
==============================================================
Files 92 92
Lines 4492 4510 +18
Branches 1189 1195 +6
==============================================================
- Hits 2335 2321 -14
- Misses 2154 2186 +32
Partials 3 3 ☔ View full report in Codecov by Sentry. |
| const cCreateX = await ethers.getContractAt(createxAbi, addresses.createX); | ||
|
|
||
| // Generate encoded salt (deployer address || crossChainProtectionFlag || bytes11(keccak256(rewardToken, gauge))) | ||
| const addressDeployerBytes20 = ethers.utils.hexlify( |
There was a problem hiding this comment.
Can we separate the salt creation code into a deploy utilities source file? It is going to be used in many deploy files and it should improve readability and risk of making mistakes.
There was a problem hiding this comment.
Done in this commit: f2bf316
| const initData = cCurvePoolBooster.interface.encodeFunctionData( | ||
| "initialize(address,uint16,address)", | ||
| // --- Deploy implementation --- // | ||
| const cachedInitCodeImpl = ethers.utils.concat([ |
There was a problem hiding this comment.
Pretty cool that you've made this work with the IMPL contract as well. Also nice to see first hand how the constructor parameters are just encoded and appended to the contract bytecode.
Just wanted to leave a remark, that we don't really need matching implementation addresses cross chain. And in favor of less complexity we shouldn't probably do it in the future. I am fine though leaving this one is as it is
There was a problem hiding this comment.
Indeed, matching implementation doesn't bring any value, 100% agree. (it's just even more satisfying).
Since I'd already started, I thought I might as well continue with the same method, in order to have an exemple of it done once (if for any reason we could need it in the futur).
I prefer to keep it like this for this implementation, but 100% agree on just using CreateX for Proxy for next time 👍
| const initializeImplem = cCurvePoolBooster.interface.encodeFunctionData( | ||
| "initialize(address,uint16,address,address)", | ||
| [ | ||
| addresses.base.multichainStrategist, // strategist |
There was a problem hiding this comment.
we should create another entry in address.js:
addresses.arbitrumOne.multichainStrategist = "0x...";
or rather set it under a non chain variable with a comment :
// works on Ethereum, Arbitrum, Base, Optimism
addresses.multichainStrategist = "0x...";
There was a problem hiding this comment.
Yes I used directly the one available, my bad.
But indeed, I could be nice to have another entry in address.js.
I'm personally in favor of addresses.multichainStrategist = "0x...";, but no problem with the chain specific one.
There was a problem hiding this comment.
agreed addresses.multichainStrategist is better
| "initialize(address,address,bytes)", | ||
| [ | ||
| implementationAddress, // implementation | ||
| addresses.mainnet.Timelock, // governor |
There was a problem hiding this comment.
we don't have a timelock on Arbitrum yet right?
There was a problem hiding this comment.
Indeed, I just copy/pasted the code from mainnet to be sure that this doesn't break cross-chain replication.
However we can setup another owner of the proxy in Arbitrum, it will not change deployment address.
And, yes, we don't have Timelock on arbitrum. Any suggestion for which address should I use instead?
There was a problem hiding this comment.
5/8 multisig on Arbitrum would be a good one: https://docs.originprotocol.com/registry/contracts#arbitrum
| @@ -20,7 +20,7 @@ describe("ForkTest: CurvePoolBooster", function () { | |||
|
|
|||
There was a problem hiding this comment.
lets also add Arbitrum fork tests and verify the initial state works. Ideally, we would want to verify that we can execute the closeCampaing function. Though I am not sure how complex it is to setup preconditions for it
There was a problem hiding this comment.
Yep, I'll add tests there too.
However we can try to add a test for closeCampaing, but it will not really test the complete logic, as we dont have the bridging message tech. But better than nothing 👍
There was a problem hiding this comment.
Can we create the campaign on Arbitrum and then also try to close it on Arbitrum? Or does the creation of the campain need to originate from the Mainnet?
There was a problem hiding this comment.
In our case the campaign originate from Mainnet.
However, even if we deployed it on Arbitrum, from what I've understand, they still need to bridge the message.
Because the main entry to create a bribe is not directly on VotemarketV2 but using the CampaignRemoteManager contract, which will bridge message.
So I think this is something that we will need to test in prod too.
…nProtocol/origin-dollar into clement/pool-booster-curve-upgrade
clement-ux
merged commit 6627d35
into
clement/pool-booster-curve
Description
On VotemarketV2, when a campaign has ended, the campaign manager may invoke the
closeCampaign()function to claim all non-claimed bribe tokens. This action is not strictly mandatory, as it can also be performed by the StakeDAO team. However, this pull request aims to introduce this functionality for improved accessibility.Note: No tests are added for this one as it will require complex cross-chain messaging setup.
Dependencies
The following deployed implementation doesn't have this functionality.
Code Change Checklist
To be completed before internal review begins:
Internal review: