refactor(contracts): cleanup unused scripts and organize ts code

[ctrlc03]

• Remove unused scripts in the contracts folder
• refactor and add typedoc comments to the typescript code
• ensure we can pass custom signers to the deployment functions.

Related to #843

[netlify]

✅ Deploy Preview for maci-typedoc ready!

Name	Link
🔨 Latest commit	458bc6b
🔍 Latest deploy log	https://app.netlify.com/sites/maci-typedoc/deploys/658160b7c6fc250008f7772a
😎 Deploy Preview	https://deploy-preview-909--maci-typedoc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[kittybest]

maci/contracts/tests/utils.ts

Line 129 in efe8d5f

export const testIncompleteSubtree = async (aq: AccQueue, contract: AccQueueContract): Promise<void> => {

Should the contract here be changed to aqContract to keep consistence with other testFill functions?

[kittybest]

maci/contracts/tests/AccQueue.test.ts

Line 38 in efe8d5f

await expect(aqContract.getSubRoot(0)).to.be.revertedWithCustomError(aqContract, "InvalidIndex");

This has been tested in testEmptyUponDeployment above, not sure should it be tested again?
If we just wanna test the contract couldn't get an non-existing index, maybe choose another index instead of 0?

[kittybest]

maci/contracts/tests/utils.ts

Line 188 in efe8d5f

export const testEnqueue = async (

For me, I would expect this function testing if enqueueing leaves works well. Not sure why we need to insert another subtree? Isn't it testing two things? (The leaves could be successfully inserted, and if the tree is full, there will be another subtree) or actually they are the same thing? (please tell me if I understand wrong)

[ctrlc03]

maci/contracts/tests/utils.ts

Line 129 in efe8d5f

export const testIncompleteSubtree = async (aq: AccQueue, contract: AccQueueContract): Promise<void> => {

Should the contract here be changed to aqContract to keep consistence with other testFill functions?

Sure, changed that for all accQueue test functions

[ctrlc03]

maci/contracts/tests/AccQueue.test.ts

Line 38 in efe8d5f

await expect(aqContract.getSubRoot(0)).to.be.revertedWithCustomError(aqContract, "InvalidIndex");

This has been tested in testEmptyUponDeployment above, not sure should it be tested again? If we just wanna test the contract couldn't get an non-existing index, maybe choose another index instead of 0?

Hold you thoughts on the tests, let's write everything that is not clear down so we can fix in another PR 🙂
Here tbh I just moved these test cases around, did not check them nor wrote them myself so would have to take some time to answer your questions.
Happy for you to work on improving these tests once we done with the other priorities if you'd like @kittybest

[kittybest]

maci/contracts/tests/AccQueue.test.ts

Line 467 in 282ff28

it("mergeSubRoots() should fail on an empty AccQueue", async () => {

The test here is actually being tested also in the edge tests

maci/contracts/tests/AccQueue.test.ts

Line 207 in 282ff28

it("Should not be possible to merge if empty", async () => {

[kittybest]

Some typos:

maci/contracts/tests/Tally.test.ts

Line 183 in 724f18e

it("tallyVotes() shuold revert when votes have already been tallied", async () => {

should*

maci/contracts/tests/Poll.test.ts

Line 112 in 724f18e

it("shold not publish a message after the voting period", async () => {

should* again

[kittybest]

maci/contracts/tests/Tally.test.ts

Line 129 in 724f18e

const onChainPackedVals = BigInt(await tallyContract.genTallyVotesPackedVals(users.length, 0, tallyBatchSize));

Just curious that should we make arguments in packTallyVotesSmallVals function same as the tallyContract.genTallyVotesPackedVals?

[kittybest]

maci/contracts/tests/MessageProcessor.test.ts

Line 148 in 724f18e

expect(packedVals.toString(16)).to.eq(onChainPackedVals.toString(16));

why we use toString(16) here while using just toString() elsewhere?

[kittybest]

maci/contracts/tests/MaciOverflow.test.ts

Line 28 in 724f18e

it("SignUps - should not overflow", async () => {

what kind of overflow are we testing here?

[kittybest]

maci/contracts/tests/MaciOverflow.test.ts

Lines 54 to 55 in 724f18e

	maxMessages: 3125,
	maxVoteOptions: 25,

should we add how the numbers calculated in the comments?

[kittybest]

maci/contracts/package.json

Lines 16 to 25 in 724f18e

	"test-maci": "hardhat test ./tests/MACI.test.ts",
	"test-poll": "hardhat test ./tests/Poll.test.ts",
	"test-messageProcessor": "hardhat test ./tests/MessageProcessor.test.ts",
	"test-tally": "hardhat test ./tests/Tally.test.ts",
	"test-hasher": "hardhat test ./tests/Hasher.test.ts",
	"test-utilities": "hardhat test ./tests/Utilities.test.ts",
	"test-signupGatekeeper": "hardhat test ./tests/SignUpGatekeeper.test.ts",
	"test-verifier": "hardhat test ./tests/Verifier.test.ts",
	"test-accQueue": "hardhat test ./tests/AccQueue.test.ts",
	"test-accQueueBenchmark": "hardhat test ./tests/AccQueueBenchmark.test.ts"

I think we miss the test-hasherBenchmarks and test-maciOverflow here?

[kittybest]

maci/contracts/tests/MaciOverflow.test.ts

Line 30 in 724f18e

users[0].pubKey.asContractParam() as { x: BigNumberish; y: BigNumberish },

maci/contracts/tests/SignUpGatekeeper.test.ts

Line 73 in 724f18e

user.pubKey.asContractParam() as { x: BigNumberish; y: BigNumberish },

maci/contracts/tests/MACI.test.ts

Line 65 in 724f18e

user.pubKey.asContractParam() as { x: BigNumberish; y: BigNumberish },

I tried to remove the as { x: BigNumberish; y: BigNumberish } part to make it clearer, and it still works, so I think we could remove this?

[kittybest]

maci/contracts/tests/MACI.test.ts

Lines 230 to 233 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

Do you know why put the first BigInt(0) in the array, while adding other BigInt(0) in the for loop?
Same as the below parts:

maci/contracts/tests/MessageProcessor.test.ts

Lines 85 to 88 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

maci/contracts/tests/Poll.test.ts

Lines 72 to 75 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

maci/contracts/tests/Tally.test.ts

Lines 84 to 87 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

[kittybest]

Do we test to prevent accidentally input negative numbers somewhere?
And I found most of the gasLimit are different, how are they decided?

[kittybest]

maci/contracts/tests/MACI.test.ts

Line 246 in 724f18e

await expect(pollContract.init()).to.be.reverted;

another question, why we need the init() function public for poll contract? it seems to be inited once the maci contract call deployPoll().

[ctrlc03]

Do we test to prevent accidentally input negative numbers somewhere? And I found most of the gasLimit are different, how are they decided?

All numeric variables we use in the smart contracts are uint so no negative values can be accepted.
Honestly I don't know how they decided about the gas limits for the tests, probably just put what worked at the time to get the test through (some being more expensive computations than others and requiring more gas to complete). I'll work today on refactoring the actual tests and address some of the points you brought up about the tests

[ctrlc03]

maci/contracts/tests/MACI.test.ts

Line 246 in 724f18e

await expect(pollContract.init()).to.be.reverted;

another question, why we need the init() function public for poll contract? it seems to be inited once the maci contract call deployPoll().

So you suggest we make it external instead? that would definitely work

[ctrlc03]

maci/contracts/tests/MACI.test.ts

Lines 230 to 233 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

Do you know why put the first BigInt(0) in the array, while adding other BigInt(0) in the for loop?
Same as the below parts:

maci/contracts/tests/MessageProcessor.test.ts

Lines 85 to 88 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

maci/contracts/tests/Poll.test.ts

Lines 72 to 75 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

maci/contracts/tests/Tally.test.ts

Lines 84 to 87 in 724f18e

	const messageData = [NOTHING_UP_MY_SLEEVE, BigInt(0)];
	for (let i = 2; i < 10; i += 1) {
	messageData.push(BigInt(0));
	}

Not sure, seems unnecessary, I'll address this when I work on the test suite

[ctrlc03]

maci/contracts/tests/MaciOverflow.test.ts

Lines 54 to 55 in 724f18e

	maxMessages: 3125,
	maxVoteOptions: 25,

should we add how the numbers calculated in the comments?

definitely!

[ctrlc03]

maci/contracts/tests/MaciOverflow.test.ts

Line 28 in 724f18e

it("SignUps - should not overflow", async () => {

what kind of overflow are we testing here?

mm agree this test case is a bit vague, will refactor it when working on the test suite

[ctrlc03]

n

Good shout, definitely we should be as consistent as possible

[kittybest]

maci/contracts/tests/MACI.test.ts

Line 246 in 724f18e

await expect(pollContract.init()).to.be.reverted;

another question, why we need the init() function public for poll contract? it seems to be inited once the maci contract call deployPoll().

So you suggest we make it external instead? that would definitely work

I'm not sure whether make it as an external is better or not.
Just wonder why we need the init function for Poll.sol since it looks like just setting up isInit to true? and if it's called more than once, the call would be reverted, but currently I think the only one who will call this is the PollFactory? so why not just put the command of set isInit to true in the constructor, removing the possibility of user accidentally calling poll.init()?

[samajammin]

• ensure we can pass custom signers to the deployment functions.

Curious, why do we want this?

[kittybest]

maci/contracts/ts/deploy.ts

Lines 350 to 360 in 458bc6b

	export const deployMaci = async (
	signUpTokenGatekeeperContractAddress: string,
	initialVoiceCreditBalanceAddress: string,
	verifierContractAddress: string,
	topupCreditContractAddress: string,
	signer?: Signer,
	stateTreeDepth = 10,
	quiet = false,
	): Promise<IDeployedMaci> => {
	const { PoseidonT3Contract, PoseidonT4Contract, PoseidonT5Contract, PoseidonT6Contract } =
	await deployPoseidonContracts(signer, quiet);

Just curious about why we assign the Poseidon contracts in the deploy function arguments for other contracts deploying, such as deploySubsidy, deployTally, etc, while we deploy Poseidon and link the libraries independently in the deployMaci function?

[kittybest]

maci/contracts/ts/genEmptyBallotRootsContract.ts

Line 16 in 458bc6b

for (let i = 1; i < 6; i += 1) {

I think we could add description here explaining why running for loop from 1 to 6?

[ctrlc03]

• ensure we can pass custom signers to the deployment functions.

Curious, why do we want this?

This opens up more testing scenarios, where you can deploy smart contracts from different addresses rather than all from the same. Also you could use the signer of this custom deployer class

maci/contracts/ts/deployer.ts

Line 20 in 458bc6b

this.signer = new Wallet(privateKey, this.provider);

which could fit other use cases

[ctrlc03]

• ensure we can pass custom signers to the deployment functions.

Curious, why do we want this?

maci/contracts/tests/MACI.test.ts

Line 246 in 724f18e

await expect(pollContract.init()).to.be.reverted;

another question, why we need the init() function public for poll contract? it seems to be inited once the maci contract call deployPoll().

So you suggest we make it external instead? that would definitely work

I'm not sure whether make it as an external is better or not. Just wonder why we need the init function for Poll.sol since it looks like just setting up isInit to true? and if it's called more than once, the call would be reverted, but currently I think the only one who will call this is the PollFactory? so why not just put the command of set isInit to true in the constructor, removing the possibility of user accidentally calling poll.init()?

So the problem with that is that we need an init function inside Poll because there is a initialization step we must do before it is usable, but cannot be done inside the constructor (at this time).
Basically we first deploy a Poll contract inside the PollFactory contract. Then we deploy an AccQueue instance, and then we transfer ownership of the AccQueue instance to the Poll contract. For that, we need the address of the Poll contract which we only have after deployment (of course there are other patterns in Solidity which can be used to deploy smart contracts and for instance with create2 it's possible to know the address before the deployment, though this is not something we have implemented atm and within the scope of this refactoring). As you can see, inside the init function of the Poll we enqueue a message into the messageAq instance, which we can only do when the Poll is the owner of messageAq.