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

OZ Improvements #847

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

OZ Improvements #847

wants to merge 33 commits into from

Conversation

peersky
Copy link

@peersky peersky commented Apr 19, 2024

This PR contains improvements we did on OpenZeppelin side to use comet repo and particularly spider tool useful in monitoring stack CI/CD pipeline.

Changes consist of:

  • Replaced hardcoded URL in favor of BASE_RPC_URL environment variable for base network
  • Replaced hardcoded URL in favor of SCROLL_RPC_URL environment variable for scroll network
  • Changed Scroll rpc endpoint to use SCROLLSCAN_KEY instead of ETHERSCAN_KEY
  • Added contract creation code scraping strategy to pullFirstInternalTransactionForContract

Merging this PR implies that maintainers must agree to update env variables in repository:

  • set BASE_RPC_URL
  • set SCROLL_RPC_URL
  • set SCROLLSCAN_KEY

Motivation

  • We are using other RPC providers to crawl data and using generic <NETWORK>_RPC_URL allows us easier integrate
  • We've observed highly unstable operation of Scroll network crawling
  • We've observed highly unstable internal transaction crawling often failing

It resolves:

It surpasses:

@peersky peersky changed the title OZ Monitoring Improvements OZ Improvements Apr 19, 2024
@kevincheng96
Copy link
Contributor

@peersky thanks for making these changes. The changes LGTM, but you'll need to also update the Github workflow files to use these new env vars. Places like here and here.

@peersky
Copy link
Author

peersky commented Apr 23, 2024

@peersky thanks for making these changes. The changes LGTM, but you'll need to also update the Github workflow files to use these new env vars. Places like here and here.

added

@peersky
Copy link
Author

peersky commented May 14, 2024

@kevincheng96 bump

Copy link
Contributor

@kevincheng96 kevincheng96 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. One comment about a LOC that looks accidentally removed.

The secrets for the new env vars are now set up in the repo as well.

.github/workflows/enact-migration.yaml Show resolved Hide resolved
@peersky peersky requested a review from kevincheng96 May 21, 2024 19:33
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.

2 participants