Skip to content

Feature: Update all capital pool unit tests to use ethers instead of truffle/web3 #584

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

Jump to bottom
Merged
merged 2 commits into from
Jan 4, 2023
Merged

Feature: Update all capital pool unit tests to use ethers instead of truffle/web3 #584

merged 2 commits into from
Jan 4, 2023

Conversation

0xdewy
Copy link
Contributor

@0xdewy 0xdewy commented Dec 22, 2022 •
edited
Loading

Context

No ticket number for this one.

Changes proposed in this pull request

I noticed the pool unit tests were still using truffle/web3 so I updated them before writing more tests in the upgrade-capital-pool PR

Test plan

No tests were added

Checklist

  • Rebased the base branch
  • Attached corresponding Github issue
  • Prefixed the name with the type of change (i.e. feat, chore, test)
  • Performed a self-review of my own code
  • Followed the style guidelines of this project
  • Made corresponding changes to the documentation
  • Didn't generate new warnings
  • Didn't generate failures on existing tests
  • Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

  • 🍰 = Nice to have but not essential.
  • 💡 = Suggestion or a comment based on personal opinion
  • 🔨 = I believe this should be changed.
  • 🤔 = I don’t understand something, do you mind giving me more context?
  • 🚀 = Feedback

@0xdewy 0xdewy marked this pull request as ready for review December 22, 2022 00:07
@0xdewy 0xdewy changed the title Update all unit tests to use ethers instead of truffle/web3 Update all capital pool unit tests to use ethers instead of truffle/web3 Dec 22, 2022
@0xdewy 0xdewy requested review from shark0der and danoctavian and removed request for shark0der December 22, 2022 17:31
@0xdewy 0xdewy changed the title Update all capital pool unit tests to use ethers instead of truffle/web3 Feature: Update all capital pool unit tests to use ethers instead of truffle/web3 Dec 22, 2022
0xdewy
0xdewy commented Dec 22, 2022
View reviewed changes
lib/constants.js
const PoolAddressParamType = {
swapOperator: hex('SWP_OP'),
priceFeedOracle: hex('PRC_FEED'),
swapOperator: hex('SWP_OP'.padEnd(8, '\0')),
Copy link
Contributor Author

@0xdewy 0xdewy Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was reverting without the 0 padding. I wasn't sure if the other ones were ok, so I added it to the others also.

@roxdanila roxdanila force-pushed the feature/upgrade-capital-pool-assets branch from 98c88af to f165501 Compare January 3, 2023 12:24
danoctavian
danoctavian reviewed Jan 3, 2023
View reviewed changes
contracts/modules/capital/Pool.sol Outdated

// Transfer cover assets. Start from 1 (0 is ETH)
uint coverAssetsCount = coverAssets.length;
uint coverAssetsCount = coverAssets.length; // substract 1 as ETH is skiped
Copy link
Contributor

Choose a reason for hiding this comment

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

typo skiped -> skipped

Also you are starting with an index of 1 below so it seems to me you shouldn't substract for ETH to be skipped. and you actually don't so the comment is misleading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice catch! Yup that comment didn't get deleted from the old code. Will fix that today.

danoctavian
danoctavian reviewed Jan 3, 2023
View reviewed changes
contracts/modules/capital/Pool.sol
}


constructor (
Copy link
Contributor

Choose a reason for hiding this comment

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

the constructor is quite complex now.

It deserves its own unit test at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I agree on this point. I wanted to update to ethers before writing the tests though, so maybe I will add them to the PR this is merged into?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah sure where it make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do this in the other PR right

danoctavian
danoctavian suggested changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

small change requests, mainly to add a dedicated constructor unit test

@0xdewy 0xdewy force-pushed the feature/update-pool-tests-to-ethers branch from d72e2ba to e12a1d0 Compare January 3, 2023 16:27
@0xdewy 0xdewy force-pushed the feature/update-pool-tests-to-ethers branch from e12a1d0 to 7473aaf Compare January 3, 2023 22:47
0xdewy
0xdewy commented Jan 3, 2023
View reviewed changes
test/unit/StakingPool/depositTo.js
const { firstActiveTrancheId, maxTranche } = await getTranches(daysToSeconds(0), DEFAULT_GRACE_PERIOD);

const tranches = Array(maxTranche - firstActiveTrancheId + 1)
const tranches = Array(maxTranche - firstActiveTrancheId)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this broke during this PR, but the StakingPool was reverting on this test when using 8 tranches. The js variable firstActiveTrancheId isn't the current firstActiveTrancheId in the contract, but is the first one available for this deposit, relative to period and gracePeriod. It was iterating past the max active tranches for this reason.

@0xdewy 0xdewy mentioned this pull request Jan 4, 2023
Closed
9 tasks
@0xdewy 0xdewy requested a review from danoctavian January 4, 2023 00:05
danoctavian
danoctavian reviewed Jan 4, 2023
View reviewed changes
test/utils/errors.js
@@ -0,0 +1,45 @@
// Used for generic compiler inserted panics.
const COMPILER = 0x00;
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming these were not defined anywhere in ethers

great work on having them here

danoctavian
danoctavian approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

LGTM good cleanup

some minor comment

@0xdewy 0xdewy merged commit c0fafb1 into feature/upgrade-capital-pool-assets Jan 4, 2023
@0xdewy 0xdewy deleted the feature/update-pool-tests-to-ethers branch January 4, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shark0der shark0der Awaiting requested review from shark0der

1 more reviewer

@danoctavian danoctavian danoctavian approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0xdewy @danoctavian