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.

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

Migrate AccessManager tests to ethers #4710

Merged

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Oct 28, 2023

Similar to #4689, #4657 and #4694.

Tests for the AccessManager were migrated to ethers along with the constants.js, namespaced-storage.js and access-manager.js helpers

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2023

⚠️ No Changeset found

Latest commit: 67d5b41

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ernestognw ernestognw marked this pull request as ready for review October 29, 2023 08:16
@ernestognw ernestognw requested a review from Amxx November 6, 2023 14:17
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Partial review, (6/10 files)

test/access/Ownable.test.js Outdated Show resolved Hide resolved
test/access/Ownable.test.js Outdated Show resolved Hide resolved
test/access/manager/AccessManaged.test.js Outdated Show resolved Hide resolved
test/access/manager/AccessManaged.test.js Outdated Show resolved Hide resolved
});

it('panics in short calldata', async function () {
// We avoid adding the `restricted` modifier to the fallback function because other tests may depend on it
// being accessible without restrictions. We check for the internal `_checkCanCall` instead.
await expectRevert.unspecified(this.managed.$_checkCanCall(other, '0x1234'));
await expect(this.managed.$_checkCanCall(this.roleMember, '0x1234')).to.be.reverted;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a way to check that the revert was a panic. We should do that.

Copy link
Collaborator

@Amxx Amxx Nov 6, 2023

Choose a reason for hiding this comment

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

Note, if we don't specify the panic code, it will work just like we want. Sources: https://github.com/NomicFoundation/hardhat/blob/main/packages/hardhat-chai-matchers/src/internal/reverted/revertedWithPanic.ts

Suggested change
await expect(this.managed.$_checkCanCall(this.roleMember, '0x1234')).to.be.reverted;
await expect(this.managed.$_checkCanCall(this.roleMember, '0x1234')).to.be.revertedWithPanic();

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't work but I'm not sure why. If done like this, we get an error saying that it reverted without a reason:

AssertionError: Expected transaction to be reverted with some panic code, but it reverted without a reason

Array slices don't specify if they panic in the docs, but the section with the panic codes does mention it panics for "... an array slice at an out-of-bounds or negative index"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interresting. Lets leave it like that for now. I'll try to come up with a minimum reproductive example and report it somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened this ticket. Seems to be a vague interpretation of what the panic code means. Seems to be referring to only accessing an array out of bounds but it does mention array slices in the docs.

Let's see what they say

test/access/manager/AccessManaged.test.js Outdated Show resolved Hide resolved
test/access/manager/AccessManaged.test.js Outdated Show resolved Hide resolved
test/access/manager/AccessManaged.test.js Outdated Show resolved Hide resolved
test/helpers/access-manager.js Outdated Show resolved Hide resolved
test/helpers/access-manager.js Outdated Show resolved Hide resolved
@Amxx Amxx changed the title Migrate AcessManager tests to ethers Migrate AccessManager tests to ethers Nov 8, 2023
@ernestognw ernestognw merged commit cf6ff90 into OpenZeppelin:master Nov 9, 2023
17 checks passed
@ernestognw ernestognw added the tests Test suite and helpers. label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-changeset tests Test suite and helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants