Skip to content

Conversation

paterson1
Copy link
Contributor

@paterson1 paterson1 commented Nov 4, 2022

Context

Closes #282

Changes proposed in this pull request

Added tests for the constructor and initialize functions of Cover.sol. setup was changed to expose some values to make it easier to deploy a new Cover not initialized.

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

});

const cover = await Cover.deploy(
quotationData.address,
Copy link
Contributor

Choose a reason for hiding this comment

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

these values

quotationData.address
productsV1.address

are private fields. However we can try using getStorageAt https://docs.ethers.io/v5/api/providers/provider/#Provider-getStorageAt

to get the field value.

Let's try asserting them regardless using getStorageAt

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've tried with hardhat-storage-layout and couldn't find them, and then I realized that they are immutable so they are not saved to storage

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes. You're right they are not in storage they are in contract code.

Ok this is fine for now

this.quotationData = quotationData;
this.stakingPool = stakingPool;
this.coverAddress = coverAddress;
this.futureCoverNFTAddress = futureCoverNFTAddress;
Copy link
Contributor

Choose a reason for hiding this comment

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

can add a dummy productsV1 address here.

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.

Let's try to assert private fields with getStorageAt reads and a few other things

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

@danoctavian
Copy link
Contributor

Needs rebase.

@roxdanila roxdanila force-pushed the test/cover-constructor-initialization branch from ba14d59 to 087db68 Compare November 9, 2022 08:04
@roxdanila roxdanila merged commit a6aa122 into nexus-v2 Nov 9, 2022
@roxdanila roxdanila deleted the test/cover-constructor-initialization branch November 9, 2022 09:30
@roxdanila roxdanila linked an issue Nov 9, 2022 that may be closed by this pull request
4 tasks
@roxdanila roxdanila linked an issue Nov 18, 2022 that may be closed by this pull request
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V2 Cover unit tests: Cover.sol constructor + initialization
3 participants