-
Notifications
You must be signed in to change notification settings - Fork 41
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
popm/wasm: improve and tidy up Go code #144
Conversation
This splits the web/popminer/popminer.go file into 5 files: - dispatch.go: Contains all methods that can be dispatched from JS - network.go: Contains network settings - network_local.go: Contains localnet settings (behind build tag) - popminer.go: Contains the main function and core - util.go: Contains utilities for working with JS This contains a lot of miscellaneous improvements, however the notable ones are: - Invoke Promise resolve function with js.Value instead of marshaled JSON, meaning that the result can be used directly without the need to encode/decode JSON. - All errors use a common format containing the error message, a timestamp, and the debug stack, allowing for easier debugging. - Replace 'wasmPing' with 'version' - wasmPing is not very useful and was only used for testing the connection between JS and WASM. version returns useful version information and allows the same testing. - Add "Object" type that can be used to easily create JS Objects from Go. Calling Object.Value() will create a js.Value representing the object, converting each contained value to a js.Value. This also has the benefit of breaking the build if a key changes, preventing breaks in the JS API. - Move network configurations into a separate file and use constants. I think this is cleaner than the previous solution, and allows localnet to only be available when built with the 'hemi_localnet' build tag.
0983c70
to
29cb3f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks pretty reasonable to me.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am overall ok with this but would like to understand why returning a map is preferable over predefined structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do really want some sort of detection for wasm_exec.js being out of date with the version that lives in the repo. I am even ok with storing the wasm_exec.js version in the repo and comparing it agains the build machine version. But we need some non-human detection of that file's content.
Initial pass to improve and tidy up the Go code for the WASM PoP Miner. This splits the web/popminer/popminer.go file into 6 files: - api.go: Contains all API types - dispatch.go: Contains all methods that can be dispatched from JS - network.go: Contains Hemi/Bitcoin network settings - network_local.go: Contains localnet setting (with build tag) - popminer.go: Contains the main function - util.go: Contains util functions for interacting with JavaScript This contains a lot of miscellaneous improvements, however the notable ones are: - Invoke Promise resolve function with `js.Value` instead of marshaled JSON, meaning the result can be used directly without the need to decode the result as JSON. - All errors use a common type, containing 'message', 'timetamp' and 'stack', allowing for easier debugging and consistency. - 'wasmPing' method has been replaced by a 'version' method, which returns version information for the PoP miner. - All results are now individual Go structs, allowing for more consistency and preventing random breakage from changes in other packages. - Move the network configurations into a separate file and use constants. I think this is cleaner than the previous solution, and allows localnet to only be available when build with the 'hemi_localnet' build tag. - Move dispatch function to a global object to prevent collisions and other issues. The dispatch function is now accessible via a `@hemilabs/pop-miner` object. Reviewed-on: hemilabs#144 Reviewed-by: Marco Peereboom <marco@peereboom.us> Reviewed-by: ClaytonNorthey92 <clayton.northey@gmail.com>
Summary
Initial pass to improve and tidy up the Go code for the WASM PoP Miner.
This is the first of many pull requests to improve the WASM and create a TypeScript (NPM) package that can be used to run the PoP Miner.
Changes
This splits the
web/popminer/popminer.go
file into 6 files:api.go
: Contains all API typesdispatch.go
: Contains all methods that can be dispatched from JS.The majority of the code in this file was moved from
popminer.go
.network.go
: Contains Hemi/Bitcoin network settingsnetwork_local.go
: Contains localnet settings (behind build tag, build with-tags "hemi_localnet"
to enable)popminer.go
: Contains the main function and coreutil.go
: Contains utility functions for working with JS.This contains a lot of miscellaneous improvements, however the notable ones are:
Invoke Promise resolve function with
js.Value
instead of marshalled JSON, meaning that the result can be used directly without the need to encode/decode JSON.Make all errors use a common object (created by the
jsError
function) that contains the error message, a timestamp, and the debug stack, allowing for easier debugging.Replace
wasmPing
method withversion
-wasmPing
is not very useful and was only used for testing the connection between JS and WASM.version
returns useful version information and allows the same testing.Use structs for all return types. The
jsValueOf
function supports converting structs to JavaScript Objects via reflection.Move network configurations into a separate file and use constants. I think this is cleaner than the previous solution, and allows localnet to only be available when built with the 'hemi_localnet' build tag. Note: I have found the networks to be slightly confusing, as we need to interact with Hemi and Bitcoin. This "standardises" the networks in the WASM PoP miner and uses the Hemi network name, not the Bitcoin network name. If this is not useful, then this can be reverted.
Move dispatch function to under a global object to prevent collisions and other issues. - The dispatch function is now placed in a
@hemilabs/pop-miner
global object. An example of accessing the dispatch function would beglobalThis['@hemilabs/pop-miner'].dispatch(args)
. The name@hemilabs/pop-miner
is used, as it will be the name of the NPM package.Other miscellaneous changes:
cmd/popmd/popmd.go
web/Makefile
WARN
and added ability to change log level with linker flag (e.g.-X main.logLevel=TRACE
)runpopminer
tostartPoPMiner
chan struct{}
instead ofchan bool
to block application exitPlease ask any questions and provide any feedback you have.
Note: The
wasm_exec.js
file is from Go. I added it to Git due to build issues I was having with certain environments, and it allows us to update the file when we want instead of it being whatever is included the user's installed Go version.The next pull request will contain the initial TypeScript types and their documentation.