Skip to content

Conversation

@mariacarmina
Copy link
Contributor

@mariacarmina mariacarmina commented Feb 3, 2025

Fixes #402 .

Changes proposed in this PR:

  • fix for unit
  • fix for integration

@mariacarmina mariacarmina self-assigned this Feb 3, 2025
@paulo-ocean
Copy link
Contributor

paulo-ocean commented Feb 14, 2025

btw, had a quick look and don't think this is going on the right track.. we're changing a lot of things unrelated with private keys... hard coding RPCS for instance, is one.. but there are others...
have you ever looked at file src/test/utils/addresses.ts ? we have everything there needed for refactoring the tests around private keys.
tkx

@mariacarmina
Copy link
Contributor Author

btw, had a quick look and don't think this is going on the right track.. we're changing a lot of things unrelated with private keys... hard coding RPCS for instance, is one.. but there are others... have you ever looked at file src/test/utils/addresses.ts ? we have everything there needed for refactoring the tests around private keys. tkx

Ok thank you Paulo, but the purpose was to no expose private keys in the code, now we place them in a separate file... it is a bit confusing, I remeber Alex said to not share private keys anywhere, we can have them as env vars, but it is not clear the scope of the issue then.

@alexcos20 alexcos20 closed this Jan 6, 2026
@alexcos20 alexcos20 deleted the remove-private-keys-from-tests branch January 6, 2026 07:50
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request significantly enhances the security of the test suite by removing numerous hardcoded private keys directly embedded within various integration and unit test files. The approach now leverages environment variables (e.g., process.env.ANOTHER_WALLET_PRIVATE_KEY) and a Blockchain abstraction for signer management, centralizing and making key usage more secure. Test environment configuration is also streamlined through TEST_ENV_CONFIG_FILE updates. While the primary goal of removing hardcoded keys from test logic has been successfully achieved, a new private key was added to the src/test/.env.test file, continuing an existing pattern of storing test keys within the repository.

Comments:
• [WARNING][security] While this PR successfully removes hardcoded private keys from test files, the addition of ANOTHER_WALLET_PRIVATE_KEY to src/test/.env.test continues a pattern of committing private keys (even for test purposes) to the repository. It's generally best practice to avoid committing any private keys to version control. For test environments, consider generating ephemeral keys on the fly, or ensuring all private keys are sourced from truly external, .gitignored files, or securely managed CI secrets. This helps maintain a strict 'no private keys in repo' policy.
• [INFO][other] Noticed the change in the RPCS environment variable for the CI workflow. It now points only to the development chain (8996). This change helps to simplify the CI setup for tests, which is a good operational improvement.

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.

Remove explicit private keys in the code and getSigner(0) from account creation in tests

4 participants