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

popm/wasm: improve and tidy up Go code #144

Merged
merged 7 commits into from
Jun 17, 2024
Merged

Conversation

joshuasing
Copy link
Contributor

@joshuasing joshuasing commented Jun 7, 2024

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 types
  • dispatch.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 settings
  • network_local.go: Contains localnet settings (behind build tag, build with -tags "hemi_localnet" to enable)
  • popminer.go: Contains the main function and core
  • util.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 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.

  • 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 be globalThis['@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:

  • Fix typo in cmd/popmd/popmd.go
  • Added versioning to and cleaned up web/Makefile
  • Reorganised code
  • Changed default logging level to WARN and added ability to change log level with linker flag (e.g. -X main.logLevel=TRACE)
  • Renamed runpopminer to startPoPMiner
  • Use chan struct{} instead of chan bool to block application exit
  • Consistently use lowerCamelCase for method names
  • Improve some variable and function naming

Please 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.

@joshuasing joshuasing added area: popm/wasm This is a change to the WASM popm (PoP Miner) type: refactor This refactors existing functionality type: enhancement This improves existing functionality size: L This change is large (+/- <500) area: make This changes a Makefile labels Jun 7, 2024
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.
web/www/wasm_exec.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jcvernaleo jcvernaleo left a 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

Copy link
Contributor

@marcopeereboom marcopeereboom left a 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.

web/popminer/dispatch.go Show resolved Hide resolved
web/popminer/dispatch.go Show resolved Hide resolved
web/popminer/dispatch.go Outdated Show resolved Hide resolved
web/popminer/dispatch.go Outdated Show resolved Hide resolved
web/popminer/dispatch.go Show resolved Hide resolved
web/popminer/util.go Outdated Show resolved Hide resolved
Copy link
Contributor

@marcopeereboom marcopeereboom left a 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.

@joshuasing joshuasing merged commit a4685a5 into main Jun 17, 2024
4 checks passed
@joshuasing joshuasing deleted the joshua/web-popm/improve-go branch June 17, 2024 15:40
web3cryptoguy pushed a commit to web3cryptoguy/heminetwork that referenced this pull request Nov 1, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: make This changes a Makefile area: popm/wasm This is a change to the WASM popm (PoP Miner) size: L This change is large (+/- <500) type: enhancement This improves existing functionality type: refactor This refactors existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants