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

Launch local dev wallet instance from emulator #90

Merged
merged 8 commits into from
Dec 8, 2021
Merged

Launch local dev wallet instance from emulator #90

merged 8 commits into from
Dec 8, 2021

Conversation

bluesign
Copy link
Collaborator

Description

Emulator launches a local dev wallet instance.

For Flip Fest: onflow/flip-fest#11

I am planning documentation update after this, but not sure about how to unit test this.


For contributor use:

  • [+] Targeted PR against master branch

server/server.go Outdated Show resolved Hide resolved
server/walletServer.go Outdated Show resolved Hide resolved
server/walletServer.go Outdated Show resolved Hide resolved
server/walletServer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Left some comments but looking great, great work again 🚀

@sideninja
Copy link
Contributor

sideninja commented Oct 19, 2021

As far as unit testing goes, you can use httptest https://pkg.go.dev/net/http/httptest to test routing and serving of couple of files, there's nothing much besides that I believe that needs testing, no need to overdo it. You can find an example here https://github.com/bluesign/flow-emulator/blob/master/server/liveness/check_test.go#L68

@sideninja
Copy link
Contributor

@bluesign was the branch merged with state management just now? Because I see changes from that branch now.

@bluesign
Copy link
Collaborator Author

Oh I messed up probably was just trying to push to Janez about other issue, let me fix.

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

I can't review all parts, as I haven't played around with the dev wallet much, but I went through how this interacts with the emulator code and it looks good :)

looking forward to see the usage documentation.

@sideninja
Copy link
Contributor

@bluesign try to resolve conflicts so this can be marked as done. Also please add a documentation explaining how to run this, you can add it to the readme.

@sideninja
Copy link
Contributor

Last couple of small comments I have:

  • The flags support env variables by default with snake case, in your case DEV_WALLET and WALLET_PORT
  • I would change --wallet-port to --dev-wallet-port just to be sure it's clear and consistent, I always rather error on length than clarity
  • Do you think we should change default port for dev-wallet from 3000 to 8701, which is the default port used by dev-wallet and the 3000 is quite popular for local dev of apps.

These are all quite minor things so even if ignored it's a valid submission.
Great work!

@bluesign
Copy link
Collaborator Author

bluesign commented Nov 5, 2021

@sideninja thanks, I am updating now. any idea why CI giving error at check-tidy?

@sideninja
Copy link
Contributor

@sideninja thanks, I am updating now. any idea why CI giving error at check-tidy?

did you run tidy again? This kind of CI problems are usually related to go.sum

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.

3 participants