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

docs: re-introduce testing wallets docs #2635

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

maschad
Copy link
Member

@maschad maschad commented Jun 26, 2024

@maschad maschad self-assigned this Jun 26, 2024
@github-actions github-actions bot added the docs Requests pertinent to documentation label Jun 26, 2024
@maschad maschad marked this pull request as draft June 26, 2024 21:17
@maschad maschad marked this pull request as ready for review June 26, 2024 21:36
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
80.29%(+0%) 71.8%(+0%) 77.56%(+0%) 80.36%(+0%)
Changed Files:

Coverage values did not change👌.

@nedsalk
Copy link
Contributor

nedsalk commented Jun 27, 2024

I'm concerned that we are sending conflicting messages between this and launchTestNode. Anybody who was using generateTestWallet in tests should switch to launchTestNode, and anybody who was using it in app code (like in #2617) was misusing the utility. For example, the generateTestWallet utility depends on process.env.GENESIS_SECRET, an environment variable created to solve the flakiness around double spending in our tests, etc. - it wasn't designed with app/production code in mind.

If the users have a need for such an utility, we should think about how best to give them access to it. Perhaps it's not a new utility at all, but just some documentation around Wallet.generate and how it can be integrated with a custom-made stateConfig.json - something that can be put in the cookbook.

@maschad
Copy link
Member Author

maschad commented Jun 27, 2024

I'm concerned that we are sending conflicting messages between this and launchTestNode. Anybody who was using generateTestWallet in tests should switch to launchTestNode, and anybody who was using it in app code (like in #2617) was misusing the utility. For example, the generateTestWallet utility depends on process.env.GENESIS_SECRET, an environment variable created to solve the flakiness around double spending in our tests, etc. - it wasn't designed with app/production code in mind.

I agree with you that launchTestNode is a better utility, when I created this PR I hadn't found any documentation around retrieving the wallet instance, so I still felt that generateTestWallet would be useful until that was better elucidated. Reading this now I think it suffices. Perhaps I should just highlight it here.

If the users have a need for such an utility, we should think about how best to give them access to it. Perhaps it's not a new utility at all, but just some documentation around Wallet.generate and how it can be integrated with a custom-made stateConfig.json - something that can be put in the cookbook.

This documentation around Wallet.generate outlines that so I think it's useful. Perhaps we can give a recommendation to use launchTestNode as the preface on this guide, but also highlight that if a user needs to specifically just generate a wallet that can be done via this snippet.

@maschad maschad marked this pull request as draft June 29, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Requests pertinent to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reintroduce Wallet testing documentation
3 participants