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

Add Browser Support (Neutrino) #2124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

linden
Copy link

@linden linden commented Feb 18, 2024

Minor changes needed to accommodate Neutrino in the browser (lightninglabs/neutrino#295).

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Very cool! Stoked to finally properly get the neutrino+wasm train rolling 😎

@@ -23,6 +23,11 @@ func appDataDir(goos, appName string, roaming bool) string {
return "."
}

// Fallback to an empty string on js since we do not have a file system.
if goos == "js" {
Copy link
Member

Choose a reason for hiding this comment

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

Does this case need to be handled elsewhere in practice?

@@ -27,11 +27,21 @@ import (
"github.com/btcsuite/btcd/wire"
)

var ErrNotExist = errors.New("store does not exist")

// store is a basic storage interface. Either using the file system or localStorage in the browser.
Copy link
Member

Choose a reason for hiding this comment

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

godoc string should be wrapped to 80 char columns

@@ -27,11 +27,21 @@ import (
"github.com/btcsuite/btcd/wire"
)

var ErrNotExist = errors.New("store does not exist")
Copy link
Member

Choose a reason for hiding this comment

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

Missing godoc comment.

@@ -1206,7 +1213,7 @@ func (a *AddrManager) GetBestLocalAddress(remoteAddr *wire.NetAddressV2) *wire.N
// Use Start to begin processing asynchronous address updates.
func New(dataDir string, lookupFunc func(string) ([]net.IP, error)) *AddrManager {
am := AddrManager{
peersFile: filepath.Join(dataDir, "peers.json"),
store: NewStore(filepath.Join(dataDir, "peers.json")),
Copy link
Member

Choose a reason for hiding this comment

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

Should the store also be accepted as an optional argument so it can be properly initialized in the WASM/browser context?

Copy link
Member

Choose a reason for hiding this comment

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

Ah scratch that, I see the build tag usage below!

"os"
)

type Store struct {
Copy link
Member

Choose a reason for hiding this comment

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

Missing godoc comments.

buf := bytes.NewBufferString(val)

// Create a NOP closer, we have nothing to do upon close.
return io.NopCloser(buf), nil
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -14,6 +14,7 @@ require (
github.com/gorilla/websocket v1.5.0
github.com/jessevdk/go-flags v1.4.0
github.com/jrick/logrotate v1.0.0
github.com/linden/localstorage v0.0.0-20231117043609-5d94f0a86609
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can just fold in the code into the project so we don't need to add a new extertnal dep? It could live in an internal package.

I checked it out, and looks like a pretty minimal set of helper funcs.

@Roasbeef
Copy link
Member

Approved the CI run.

We should also add a new CI target to build w/ the wasm/js tag on.

@@ -62,4 +63,4 @@ retract (
v0.13.0-beta
)

go 1.17
go 1.21.2
Copy link
Member

Choose a reason for hiding this comment

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

IIRC go.mod doesn't handle minor version specification

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.

None yet

2 participants