wasm #5084
wasm #5084
Conversation
v1rtl
force-pushed
the
wasm
branch
2 times, most recently
from
9111c7e to
e46c46c
Compare
There was a problem hiding this comment.
Great job, but have a few questions/suggestions:
- Split this PR to a few separate.
- PR has a lot of refactoring like split existing logic between *_js.go and *_shared.go
But it doesn't look like renaming, it looks like 2 new files, so it also makes review harder. In future I would suggest to avoid it. - For me it is not quite clear, is the Bee browser build, is this actually a dial-only node? How is participation in overlay/DHT ensured without incoming: relay/signal/bootstrap policy? I understand, it should work somehow, but it would be great to have some doc
- What is planned to replace Prometheus in the browser build?
- how will the node behave if the peer does not support ws?
| func isWindowsService() (bool, error) { | ||
| return false, nil | ||
| } | ||
| func (c *command) initStartCmd() (err error) { |
There was a problem hiding this comment.
In cmd/start_windows.go and cmd/start_unix.go most of the logic is the same, with the only differences being the runtime setup (Windows Service + Event Log vs. Unix signal handling).
Maybe we could extract the shared logic into a common file and add platform-specific adapters?
There was a problem hiding this comment.
you can't do platform-specific compilation by putting everything in one file. go build tags apply for the whole file
| // Mutex is a drop in replacement for the sync.Mutex | ||
| // it will not lock if the context is expired | ||
| type Mutex struct { | ||
| mu chan struct{} |
There was a problem hiding this comment.
Why can't we use sync.Mutex here instead of channel?
Looks like chan was added only for ctx expiration case, but I think it is possible to handle context expiration without creating our own version of mutex.
Or I miss smth?
There was a problem hiding this comment.
this is copied from original bee source
|
|
||
| func (m noOpChainBackend) CodeAt(context.Context, common.Address, *big.Int) ([]byte, error) { | ||
| return common.FromHex(sw3abi.SimpleSwapFactoryDeployedBinv0_6_9), nil | ||
| return common.FromHex(sw3abi.SimpleSwapFactoryDeployedBinv0_6_5), nil |
There was a problem hiding this comment.
Why lower version here?
There was a problem hiding this comment.
the branch is very out of date from upstream so this is just an older version
| return f.currOffset, nil | ||
| case io.SeekEnd: | ||
| f.currOffset = -offset | ||
| return f.currOffset, nil |
There was a problem hiding this comment.
Looks like potential problem to me. We return negative offset and no error... So method which calls this one has to keep in mind, that offset might be negative.
| case a == nil || b == nil: | ||
| return false | ||
| default: | ||
| return a.Equal(b) |
There was a problem hiding this comment.
May be instead? Logic is same, but easier to read, I think
if a == nil || b == nil { return a == nil && b == nil } return a.Equal(b)
|
|
superseeded by #5300 |
4 participants
Checklist
Description
How to get this running:
gh pr checkout 5084go build -o ./dist/bee ./cmd/bee--p2p-ws-enablemake wasmto produce the WASM buildcd dist && bun i && bun run builddistand navigate tolocalhost:{port}Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):