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

refactor: make sure to use static constant variables for accounts in fixture-builder.js #29786

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

Conversation

ffmcgee725
Copy link
Member

@ffmcgee725 ffmcgee725 commented Jan 17, 2025

-- refactor: make sure to use static constant variables for accounts in fixture-builder.js;

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Run E2E tests
  2. Should pass after change introduced

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Gudahtt
Gudahtt previously approved these changes Jan 17, 2025
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

adonesky1
adonesky1 previously approved these changes Jan 17, 2025
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

jiexi
jiexi previously approved these changes Jan 17, 2025
const { CHAIN_IDS } = require('../../shared/constants/network');
const { ACCOUNT_1, ACCOUNT_2 } = require('./helpers');
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this import is causing problems. It seems that this file is being pulled into the build somehow, and adding this import is resulting in some environment variables being referenced that are not defined, causing a build failure.

Why is this part of the build 🤔

Copy link
Member

Choose a reason for hiding this comment

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

We could resolve the error by moving these account constants from helpers to a constants file. That will ensure no additional environment variable references are brought in.

But this file being in the application bundle seems like a mistake.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did suggested change in 73b8c72 🤞🏾

@Gudahtt Gudahtt dismissed their stale review January 17, 2025 17:31

Build errors

@ffmcgee725 ffmcgee725 dismissed stale reviews from jiexi and adonesky1 via 73b8c72 January 17, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review finalised - Ready to be merged
Development

Successfully merging this pull request may close these issues.

5 participants