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

Mobile companion app #2715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Mobile companion app #2715

wants to merge 1 commit into from

Conversation

peterzen
Copy link
Member

@peterzen peterzen commented Mar 17, 2024

This adds a tor hidden service, and a settings dialog that generates the QR code of the .onion URL for pairing with a mobile app.

The Android app code is under /companionapp/android. This can be loaded into Android Studio for building and running in the AVD emulator.

image
Pairing dialog

image

image

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

I was able to build the app without problems. Could you rebase? I think there's a problem with the commit it is built off of preventing me from building the client.

client/tor/build_linux.sh Outdated Show resolved Hide resolved
client/tor/binary_tor.go Show resolved Hide resolved
client/tor/tor.go Show resolved Hide resolved
Comment on lines +286 to +289

if cfg.Logger != nil {
log = cfg.Logger
}
Copy link
Member

Choose a reason for hiding this comment

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

If log is nil we should error or we will get a panic later. Will we be calling New more than once? In that case maybe we shouldn't use a shared log.

Copy link
Member Author

Choose a reason for hiding this comment

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

New is called repeatedly in webserver_test.go, this change fixes a race:
bf0a925****

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually sure this is the most elegant solution btw, suggestions appreciated.

Copy link
Member

Choose a reason for hiding this comment

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

Can you just make a new logger for every test? Is the problem using the same logger for multiple tests at the same time? I thought the logger could handle concurrent logs though...

Copy link
Member

Choose a reason for hiding this comment

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

I see the problem now, we change the logger as it is being used.

imo the correct way to solve this is to make log part of the WebServer struct and remove the package level one. It can't be used before being initiated with New anyhow, so there's no reason for it to be package level.

@JoeGruffins
Copy link
Member

JoeGruffins commented Sep 30, 2024

Looks really nice! I didn't realize we could do something like this.

Left side could use padding here:
image

Sorry resolution is so high, putting in dropdown...

more issues

I'm not seeing an app icon. I thought there was one though:
Screenshot_20240930_160958_One UI Home

Unable to make a new account for some reason:
Screenshot_20240930_161145_BisonWallet

Copy link
Contributor

@ukane-philemon ukane-philemon left a comment

Choose a reason for hiding this comment

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

So I was thinking if we can kill two birds with one stone and have the mobile companion as a flutter application. That way we can benefit both android and ios users.

@buck54321, @JoeGruffins, @peterzen what do you think?

@@ -244,6 +244,38 @@ func (s *WebServer) handleGenerateQRCode(w http.ResponseWriter, r *http.Request)
}
}

// handleGenerateQRCode is the handler for the '/generateqrcode' page request
func (s *WebServer) handleGenerateCompanionAppQRCode(w http.ResponseWriter, r *http.Request) {

Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this space

func (s *WebServer) handleGenerateCompanionAppQRCode(w http.ResponseWriter, r *http.Request) {

var url string

Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this space

Comment on lines +257 to +258
// Create auth token and append it to the URL for authTokenMiddleware to pick up.
// TODO save this token in the DB to make it permanent?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this token changed at any point in time?

Comment on lines +699 to +700
cm.Wait()
wg.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to block until s.ctx cancellation here.

}

android {
namespace 'org.decred.dex.dexandroid'
Copy link
Contributor

Choose a reason for hiding this comment

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

just org.decred.dex should be okay, no?

compileSdk 34

defaultConfig {
applicationId "org.decred.dex.dexandroid"
Copy link
Contributor

Choose a reason for hiding this comment

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

just org.decred.dex should be okay, no?

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.

4 participants