Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

Add some missing test cases to AccountsDepositWithdrawEndowments.. #401

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Oct 4, 2023

Explanation of the solution

Instructions on making this work

  • run yarn or yarn install to install npm dependencies

@0xNeshi 0xNeshi self-assigned this Oct 4, 2023
@@ -512,10 +512,14 @@ describe("AccountsDepositWithdrawEndowments", function () {
// setup an allowed contributor wallet to sign/send
const allowedContributor = await genWallet().address;
await impersonateAccount(allowedContributor);
await setBalance(allowedContributor, 1000000000000000000); // give it some gas money, as impersonateAccount does not
await setBalance(allowedContributor, 1000000000000000000n); // give it some gas money, as impersonateAccount does not
Copy link
Contributor Author

@0xNeshi 0xNeshi Oct 4, 2023

Choose a reason for hiding this comment

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

There were warnings that such big numbers can't be represented as integers:

Numeric literals with absolute values equal to 2^53 or greater are too large to be represented accurately as integers
image

Switched them to BigInt just to be safe

@@ -1,7 +1,7 @@
{
"compilerOptions": {
"baseUrl": ".",
"target": "es2018",
"target": "ES2020",
Copy link
Contributor Author

@0xNeshi 0xNeshi Oct 4, 2023

Choose a reason for hiding this comment

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

Needed to be able to use BigInts (see #401 (comment))

@@ -768,8 +776,22 @@ describe("AccountsDepositWithdrawEndowments", function () {
});

describe("from Non-Mature endowments", () => {
it("reverts if sender is not owner", async () => {
Copy link
Contributor Author

@0xNeshi 0xNeshi Oct 4, 2023

Choose a reason for hiding this comment

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

Added missing test cases (more below)...

We still have many test cases missing, so many of our code paths are not covered.

Tried adding solidity-coverage to our repo, but stumbled upon the the most common issue with it when using viaIR (we use it), which is sc-forks/solidity-coverage#715
If we remove viaIR to make coverage work, we get the "Stack Too Deep" error, which is just too big to tackle in this PR, so just removed the coverage tool for now (although it would be very useful to have it).

).to.be.revertedWith("Beneficiary address is not listed in maturityAllowlist");
});

it("reverts if maturity allowlist is empty", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More missing test cases

@0xNeshi 0xNeshi changed the title [WIP] Add slither analysis step Add some missing test cases to AccountsDepositWithdrawEndowments.. Oct 4, 2023
@0xNeshi 0xNeshi added the enhancement New feature or request label Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant