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

OPCM: unify assertValidContractAddress #13723

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

zhiqiangxu
Copy link
Contributor

@zhiqiangxu zhiqiangxu commented Jan 13, 2025

This PR replaces the OPCM.assertValidContractAddress with DeployUtils.assertValidContractAddress, which makes it more consistent.

(OPCM is the only contract that defines its own assertValidContractAddress function)

@zhiqiangxu zhiqiangxu requested a review from a team as a code owner January 13, 2025 07:12
@zhiqiangxu zhiqiangxu requested a review from mbaxter January 13, 2025 07:12
Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

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

Overall this LGMT and it's great to reduce redundancy.

But I have some reservations about pulling in code from a lib called DeployUtils which is stored in scripts.

I think I'd prefer to move this out of DeployUtils into something in src/libraries. Either in an existing library if a relevent one exists (nothing looked immediately obvious), or a new one if necessary.

@maurelian
Copy link
Contributor

/ci authorize ca0088e

@tynes
Copy link
Contributor

tynes commented Jan 13, 2025

Generally agree that pulling in something from scripts into something that lives onchain is a leaky abstraction. Altho we are doing "onchain scripting". Generally onboard with abstracting this into a library, feels pretty similar to the OZ isContract function

@zhiqiangxu
Copy link
Contributor Author

Btw isContract has been removed: OpenZeppelin/openzeppelin-contracts#3945

Looks like the OZ way is to just check the length.

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.

3 participants