Proposal script #50
Proposal script #50
Conversation
alexkeating
force-pushed
the
feature/proposal-script
branch
from
de8e385
to
ae10568
Compare
apbendi
force-pushed
the
feature/add-deploy-script
branch
from
61528aa
to
f412595
Compare
alexkeating
force-pushed
the
feature/add-deploy-script
branch
from
f412595
to
b2280ad
Compare
apbendi
force-pushed
the
feature/add-deploy-script
branch
from
b2280ad
to
77eb2bd
Compare
alexkeating
force-pushed
the
feature/proposal-script
branch
2 times, most recently
from
8965936
to
e5dd3d8
Compare
alexkeating
force-pushed
the
feature/proposal-script
branch
2 times, most recently
from
4511fd1
to
346dea3
Compare
test/UniStaker.integration.t.sol
Outdated
| assertEq(factory.owner(), address(v3FactoryOwner)); | ||
|
|
||
| assertEq(wbtcWethFeeProtocol, 10 + (10 << 4)); | ||
| assertEq(daiWethFeeProtocol, 10 + (10 << 4)); |
There was a problem hiding this comment.
why these numbers? can you leave a comment?
also, can these numbers be fuzzed, or would we always expect them to be fixed/hardcoded in the script?
There was a problem hiding this comment.
Right now they are hardcoded in the script. We could update the script to make these different from a testing perspective, but I don't know how much this gets us testing wise because we should already be fuzzing these values in the unit tests.
There was a problem hiding this comment.
Hey @alexkeating overall it looks great, really glad to see the gov proposal testing infrastructure in place! Left some thoughts, feel free to push back or suggest alternatives to my suggestions.
There was a problem hiding this comment.
Left some more comments @alexkeating. Thanks for bearing with multi-review cycles on the proposal scripts. Since they'll be used repeatedly over time, I want to make sure we get them right.
alexkeating
force-pushed
the
feature/proposal-script
branch
from
258c2e4
to
39f7f0f
Compare
script/ProposeFactorySetOwner.s.sol
Outdated
| ); | ||
| } | ||
|
|
||
| /// @param _newV3FactoryOwner The new factory owner which should have be the recently deployed. |
There was a problem hiding this comment.
| /// @param _newV3FactoryOwner The new factory owner which should have be the recently deployed. | |
| /// @param _newV3FactoryOwner The new factory owner, i.e. the recently deployed V3FactoryOwner. |
script/ProposeFactorySetOwner.s.sol
Outdated
| address _proposer = | ||
| vm.envOr("PROPOSER_ADDRESS", address(0x0459f41c5f09BF678D9C07331894dE31d8C22255)); | ||
|
|
||
| /// @param _newV3FactoryOwner The new factory owner which should have be the recently deployed. |
There was a problem hiding this comment.
| /// @param _newV3FactoryOwner The new factory owner which should have be the recently deployed. | |
| /// @param _newV3FactoryOwner The new factory owner i.e. the recently deployed V3FactoryOwner. |
script/ProposeFactorySetOwner.s.sol
Outdated
|
|
||
| /// @param _newV3FactoryOwner The new factory owner which should have be the recently deployed. | ||
| /// `V3FactoryOwner` | ||
| /// @dev After the UniStaker and V3FactoryOwner contracts are deployed a delegate should run this |
There was a problem hiding this comment.
| /// @dev After the UniStaker and V3FactoryOwner contracts are deployed a delegate should run this | |
| /// @dev After the UniStaker and V3FactoryOwner contracts are deployed, a delegate should run this |
There was a problem hiding this comment.
^^ also, it looks like there's a period on line 35 but the period should be on line 36
| assertEq(oldDaiWethFeeProtocol, 0); | ||
| assertEq(oldDaiUsdcFeeProtocol, 0); | ||
|
|
||
| assertEq(newWbtcWethFeeProtocol, 10 + (10 << 4)); |
There was a problem hiding this comment.
i appreciate now why we're hardcoding here, but can you leave a comment on why we're doing the addition?
There was a problem hiding this comment.
Yes, I will add a comment
|
Coverage after merging feature/proposal-script into main will be
Coverage Report
|
|||||||||||||||||||||||||
Description
Closes #25