added e2e test #13
added e2e test #13
Conversation
test/v2/core/sgETH.spec.js
Outdated
|
|
||
| // revoke deployer minter rights | ||
| // actually don't need to do this because deployer doesn't have minter role | ||
| await sgEth.removeMinter(deployer.address); |
There was a problem hiding this comment.
hmm. good spot.
ok lets not do this then.
lets instead
- make sure deployer cant mint
- no one can mint
- add minter
- make sure only minter can mint
test/v2/core/sgETH.spec.js
Outdated
| feeCalc, | ||
| sgEth.address, | ||
| wsgETH.address, | ||
| ethers.constants.AddressZero, // using dummy address |
There was a problem hiding this comment.
can you comment in what these addresses are supposed to be?
cos i forgot lol
i think being able to deploy with a zero addr will annoy auditers, so we should
- make sure its not possible to deploy with 0 addr OR
- if deployed w/ 0 addr, it fails at whatever the top most fn call would be, in our case maybe its
deposit()called by users with eth. And is caught by a deploy script or the e2e tests in the deploy scripts
test/v2/core/sgETH.spec.js
Outdated
| const wsgETH = await WSGETH.deploy(sgEth.address, 24 * 60 * 60); | ||
| await wsgETH.deployed(); | ||
|
|
||
| const feeCalc = ethers.constants.AddressZero; // not sure if it is right |
There was a problem hiding this comment.
iirc, theres 3 missing contracts here from the default deploy pipeline linked below:
https://github.com/chimera-defi/SharedDeposit/blob/main/deploy/v2/lib/onchain_actions.js#L102
-
daoFeeSplitter -> OpenZeppellin vanilla paymentsSplitter , pulled in here
SharedDeposit/deploy/v2/lib/opts.js
Line 57 in ae40f84
source:
OZ-3.3. - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v3.3/contracts/payment/PaymentSplitter.sol
Note: - this is removed from OZ master, should check to make sure its still safe. Also need to rethink this build approach since it wasnt obvious to you what this contract should be? -
Withdrawals -> https://github.com/chimera-defi/SharedDeposit/blob/main/contracts/v2/core/Withdrawals.sol
-
RewardsReceiver -> https://github.com/chimera-defi/SharedDeposit/blob/main/contracts/v2/core/RewardsReceiver.sol
There was a problem hiding this comment.
Could you let me know parameters of each contract constructor?
There was a problem hiding this comment.
you can see the constructor args here
https://github.com/chimera-defi/SharedDeposit/blob/main/deploy/v2/lib/onchain_actions.js#L102
they are defined here https://github.com/chimera-defi/SharedDeposit/blob/main/deploy/v2/lib/opts.js#L45
There was a problem hiding this comment.
What's the deposit contract? on args of minterv2?
test/v2/core/sgETH.spec.js
Outdated
| .withArgs(ethers.constants.HashZero, deployer.address, deployer.address); | ||
|
|
||
| // check auth invariants are preserved. i.e ex owner and outsiders cannot interact with the contract | ||
| await expect(sgEth.connect(deployer).transferOwnership(alice.address)).to.be.revertedWith( |
There was a problem hiding this comment.
we check deployer cant transfer to alice, but can we also make sure alice cant transfer to anyone? deployer + 0x00?
There was a problem hiding this comment.
I think alice can transfer ownership to another. do you want me to check this?
There was a problem hiding this comment.
yea?
how does the ownership work here?
in the architecture, only the admin should be able to transfer ownership, and after the deployer renounces, only a multisig / test addr that recvd ownership should be able to act to transfer ownership or call owner only fns.
|
Can we pull the e2e into its own file? |
|
So you want to move this e2e script into a new file? |
yep. |
No description provided.