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

Use hardhat-exposed to reduce the need for mocks #3666

Merged
merged 68 commits into from
Jan 3, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Aug 31, 2022

PR Checklist

  • Tests

@socket-security
Copy link

socket-security bot commented Sep 1, 2022

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

@frangio
Copy link
Contributor

frangio commented Sep 1, 2022

Inspected the above postinstall scripts and they're safe.

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Nov 16, 2022

FYI, just don't underestimate this breaking change - it will break many of my unit tests since I pull some of your (here deleted) mocks via submodules. So once you merge this PR to master and I update the submodules it will break (i.e. before you make a new release). Just wanted to quickly make you aware of this.

@frangio
Copy link
Contributor

frangio commented Nov 16, 2022

@pcaversaccio Thanks for the heads up. We don't guarantee stability of mocks at all. You should try to remove that dependency if possible.

frangio
frangio previously approved these changes Jan 2, 2023
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Ready to merge this.

contracts/utils/Base64.sol Outdated Show resolved Hide resolved
pragma solidity ^0.8.0;

import "../token/ERC20/ERC20.sol";
import {ERC20} from "../token/ERC20/ERC20.sol";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe that is the first time we use this syntax. Is that something we want to be doing more ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure. It wasn't a conscious decision, I think I went along with the syntax from a16z/erc4626-tests.

@Amxx
Copy link
Collaborator Author

Amxx commented Jan 3, 2023

LGTM

@frangio frangio enabled auto-merge (squash) January 3, 2023 14:23
@frangio frangio merged commit c1d9da4 into OpenZeppelin:master Jan 3, 2023
@gitpoap-bot
Copy link

gitpoap-bot bot commented Jan 3, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

GitPOAP: 2023 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@Amxx Amxx deleted the mocks/exposed branch January 3, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants