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

Deduplicate code in scripts/helpers and test/helpers/iterate #4895

Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 13, 2024

PR Checklist

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

@Amxx Amxx added tests Test suite and helpers. ignore-changeset labels Feb 13, 2024
Copy link

changeset-bot bot commented Feb 13, 2024

⚠️ No Changeset found

Latest commit: 75f671e

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 changed the title Deduplicate code in ./scripts/helpers and ./test/helpers/iterate Deduplicate code in scripts/helpers and test/helpers/iterate Feb 13, 2024
ernestognw
ernestognw previously approved these changes Feb 13, 2024
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

I checked where the functions are used in the scripts folder and so far:

  • chunk: Unused
  • product: Unused
  • range: In the SafeCast template
  • unique: Unused
  • zip: Unused
  • mapValues: Unused
  • capitalize: In multiple templates

Since these are used within automatic generation, these are already checked in the CI so I'm approving.

However, I wonder if it makes sense to keep the helpers in where they're used the most (i.e tests/)

@ernestognw
Copy link
Member

CI failing due to #4896. Rerunning

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 14, 2024

However, I wonder if it makes sense to keep the helpers in where they're used the most (i.e tests/)

I considered keeping all that code in test/helpers/iterate.js and have the generation script point to that. It felt strange to have "production" scripts rely on testing code... but at the same time, iterate.js sounds like a better description of what the file contains.

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 14, 2024

For the record, the "chunk" helpers was used in Forta's early days.

We wanted to relay a lot of transaction trough multicall, but not all at once (we did that in blocks of 16 so that we benefit from multicall while limiting the size of the multicall transaction)

@ernestognw
Copy link
Member

It felt strange to have "production" scripts rely on testing code...

Right, that feels weird. I think this is better anyway since we don't ship those scripts. And they're checked in the CI.

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM

@ernestognw ernestognw merged commit 96e5c08 into OpenZeppelin:master Feb 15, 2024
20 checks passed
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.

None yet

2 participants