Refactor, add miniscript parse and compilation functionality #1
Conversation
Makes it easy to add more test cases by extracting the test case values into a JSON based test vector file.
Adds two new functions to parse and compile miniscript to the Rust wrapper.
Adds the corresponding Go functions for the new Rust functions.
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Setup go ${{ env.GO_VERSION }} | ||
| uses: actions/setup-go@v3 |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: git checkout | ||
| uses: actions/checkout@v3 |
|
|
||
| jobs: | ||
| rust-compilation-check: | ||
| name: Rust compilation check check |
| # Go needs absolute directories, using the $HOME variable doesn't work here. | ||
| GOPATH: /home/runner/work/go | ||
|
|
||
| GO_VERSION: '1.22.11' |
There was a problem hiding this comment.
Better to get it from go.mod? https://github.com/actions/setup-go?tab=readme-ov-file#getting-go-version-from-the-gomod-file
| @@ -1,3 +1,6 @@ | |||
| build-wrapper: | |||
| cd wrapper && cargo build --release --target wasm32-wasip1 | |||
| cp wrapper/target/wasm32-wasip1/release/wrapper.wasm descriptors/ | |||
|
|
|||
| unit: | |||
There was a problem hiding this comment.
I prefer unit-test or run-unit-tests, or to drop this completely as go test ./... is easy enough on its own.
| _, err := NewDescriptor("tr([e81a5744/48'/0'/0'/2']xpub6Duv8Gj9gZeA3sUo5nUMPEv6FZ81GHn3feyaUej5KqcjPKsYLww4xBX4MmYZUPX5NqzaVJWYdYZwGLECtgQruG4FkZMh566RkfUT2pbzsEg/<0;1>/*,and_v(v:pk([3c157b79/48'/0'/0'/2']xpub6DdSN9RNZi3eDjhZWA8PJ5mSuWgfmPdBduXWzSP91Y3GxKWNwkjyc5mF9FcpTFymUh9C4Bar45b6rWv6Y5kSbi9yJDjuJUDzQSWUh3ijzXP/<0;1>/*),older(65535)))#lg9nqqha") | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "Invalid checksum") | ||
| type DerivationTestVector struct { |
There was a problem hiding this comment.
Can unexport this, derivationTestVector
| @@ -22,6 +22,19 @@ const ( | |||
| NetworkRegtest Network = 2 | |||
| ) | |||
|
|
|||
| func ParseNetwork(network string) (Network, error) { | |||
There was a problem hiding this comment.
move this to descriptors_test.go? These strings are specific to the unit test vectors only.
| type wasmInstance struct { | ||
| mod *wasmModule | ||
| ptr uint64 | ||
| drop func() | ||
| } | ||
|
|
||
| type Descriptor struct { | ||
| mod *wasmModule | ||
| ptr uint64 | ||
| close func() | ||
| wasmInstance | ||
| } |
There was a problem hiding this comment.
What's the purpose of this transformation? I am not a big fan of struct embedding, in my experience it tends to cause more trouble than is worth it. If your goal was to encapsulate the mod/ptr/drop parts, please make it a proper struct field like wasmInstance wasmInstance
| OpCodes uint64 | ||
| } | ||
|
|
||
| func ParseMiniscript(script string) (*Miniscript, error) { |
There was a problem hiding this comment.
What is exactly the purpose of these two functions that cannot be accomplished with the Descriptor struct? Can't you call NewDescriptor() and to add methods to get the pkScript.
This one specifically only returns types and the op code count, but for what purpose?
Also this seems to hardcode the SegWit context, which seems arbitrary.
| }, nil | ||
| } | ||
|
|
||
| func CompileMiniscript(script string) ([]byte, error) { |
There was a problem hiding this comment.
"Compile" is a bit overloaded as a name (and reminds of the policy compiler). MiniscriptToPkScript or something like that would be good. But again, why do we need this when we have NewDescriptor() which can serve the same purpose?
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: build wrapper | ||
| run: make build-wrapper |
There was a problem hiding this comment.
Can we add a check here that wrapper.wasm was not changed by this, to ensure the comitted wrapper.wasm file is always up to date?
|
Thanks for the quick review and feedback! My main intention with this is to eventually be able to replace btcsuite/btcd#1987 with this library. |
Does some refactoring of unit tests, then adds new functionality from the Rust library for parsing and compiling miniscript expressions, including unit tests.