Skip to content
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.

Sign up for GitHub

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

added e2e test #13

Merged
merged 7 commits into from
May 31, 2024
Merged

added e2e test #13

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e102c9d
added e2e test
devlancer412 May 28, 2024
ce17f8b
added comment
devlancer412 May 28, 2024
e34c0b9
fix
devlancer412 May 28, 2024
df46a1f
fix from feedback
devlancer412 May 28, 2024
0bd9820
changed comment
devlancer412 May 28, 2024
a4c4ec0
e2e test finished
devlancer412 May 29, 2024
47fc6b9
split e2e test to new file
devlancer412 May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions test/v2/core/sgETH.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,44 @@ describe("SgETH.sol", () => {
.and.to.be.emit(sgEth, "RoleRevoked")
.withArgs(ethers.constants.HashZero, deployer.address, deployer.address);
});

it("e2e test", async () => {
// deploy sgeth
const WSGETH = await ethers.getContractFactory("WSGETH");
const wsgETH = await WSGETH.deploy(sgEth.address, 24 * 60 * 60);
await wsgETH.deployed();

const feeCalc = ethers.constants.AddressZero; // not sure if it is right
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. daoFeeSplitter -> OpenZeppellin vanilla paymentsSplitter , pulled in here

    SharedDeposit/deploy/v2/lib/opts.js

    Line 57 in ae40f84

    daoFeeSplitter: "PaymentSplitter",

    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?

  2. Withdrawals -> https://github.com/chimera-defi/SharedDeposit/blob/main/contracts/v2/core/Withdrawals.sol

  3. RewardsReceiver -> https://github.com/chimera-defi/SharedDeposit/blob/main/contracts/v2/core/RewardsReceiver.sol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you let me know parameters of each contract constructor?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the deposit contract? on args of minterv2?

const numValidators = 1000;
const adminFee = 0;

const addresses = [
feeCalc,
sgEth.address,
wsgETH.address,
ethers.constants.AddressZero, // using dummy address
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. make sure its not possible to deploy with 0 addr OR
  2. 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

ethers.constants.AddressZero, // using dummy address
];

// add secondary minter contract / eoa
const Minter = await ethers.getContractFactory("SharedDepositMinterV2");
const minter = await Minter.deploy(numValidators, adminFee, addresses);
await minter.deployed();

// revoke deployer minter rights
// actually don't need to do this because deployer doesn't have minter role
await sgEth.removeMinter(deployer.address);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. good spot.
ok lets not do this then.
lets instead

  1. make sure deployer cant mint
  2. no one can mint
  3. add minter
  4. make sure only minter can mint

// add secondary owner
// revoke deployer admin rights
await expect(sgEth.transferOwnership(minter.address))
.to.be.emit(sgEth, "RoleGranted")
.withArgs(ethers.constants.HashZero, minter.address, deployer.address)
.and.to.be.emit(sgEth, "RoleRevoked")
.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(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we check deployer cant transfer to alice, but can we also make sure alice cant transfer to anyone? deployer + 0x00?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think alice can transfer ownership to another. do you want me to check this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. got it

`AccessControl: account ${deployer.address.toLowerCase()} is missing role ${ethers.constants.HashZero}`,
);
});
});