-
Notifications
You must be signed in to change notification settings - Fork 114
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
test: add .env.example file with its description to integration-tests Readme.md #58
Conversation
Visit the preview URL for this PR (updated for commit 6e62c51):
(expires Thu, 26 Oct 2023 13:20:20 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: e508f9012944951194447cb8885950b451a24403 |
|
||
import type { HardhatUserConfig } from "hardhat/types"; | ||
|
||
const envFilePath = `${__dirname}/.env`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't do any file existence checks. We should just have a fallback to Wallets.richWalletPrivateKey
. No logic is needed here in this file except loading dotenv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallback logic should be used in place where process.env.WALLET_PRIVATE_KEY
is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Romsters updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally okay with the PR, but let's get here an approval from Roman since there were some comments
|
||
import type { HardhatUserConfig } from "hardhat/types"; | ||
|
||
if (!process.env.WALLET_PRIVATE_KEY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this check and reassigning env vars is a bad pattern. I meant you can add a fallback in the place where it is used.
Like:
const wallet = new Wallet(process.env.WALLET_PRIVATE_KEY || Wallets.richWalletPrivateKey);
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thank you. @Romsters
What ❔
env.example file with Readme description to integration-tests
Why ❔
To improve UX and avoid issues in preparing a local env
Checklist
env.example
Readme.md
[ + ] PR title corresponds to the body of PR (we generate changelog entries from PRs).
[ n\a ] Tests for the changes have been added / updated.
[ + ] Documentation comments have been added / updated.