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

Wasm bindings - a start #265

Merged
merged 16 commits into from Jan 11, 2023
Merged

Wasm bindings - a start #265

merged 16 commits into from Jan 11, 2023

Conversation

michaelneale
Copy link
Contributor

@michaelneale michaelneale commented Dec 9, 2022

This is the first pass add adding wasm binary (and an example web app that uses it from javascript) to ssi-sdk.

thanks to @mistermoe for starting this and working out how it can work: https://github.com/TBD54566975/ssi-sdk-wasm-bindings-examples

@michaelneale michaelneale changed the title Wasm Wasm bindings - a start Dec 9, 2022
magefile.go Outdated Show resolved Hide resolved
magefile.go Outdated Show resolved Hide resolved
wasm/main.go Outdated Show resolved Hide resolved
* This is the glue to bind the functions into javascript so they can be called
*/
func main() {
done := make(chan struct{})
Copy link
Member

Choose a reason for hiding this comment

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

curious what the difference is if this channel is removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, you need to keep the process alive.

Copy link
Member

Choose a reason for hiding this comment

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

}

// 3. Returning a richer object, converting to json and then unmarshalling to make it a js object
func makeDid(_ js.Value, args []js.Value) interface{} {
Copy link
Member

Choose a reason for hiding this comment

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

nit: makeDID can also replace interface{} with the new any alias

// 3. Returning a richer object, converting to json and then unmarshalling to make it a js object
func makeDid(_ js.Value, args []js.Value) interface{} {

pubKey, _, _ := crypto.GenerateKeyByKeyType(crypto.Ed25519)
Copy link
Member

Choose a reason for hiding this comment

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

should we pull out the errs here and below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to think about with WASM is how it interfaces with storage. It's not quite clear cut. I.e you generate the public key in WASM, but you'll now how to figure out a way to transfer and serialize the key to the application handling the storage. That transfer always felt a little "iffy" to me. I.e you've now sent your private key to your application, and that may be a security risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andorsk I am not that worried about that at the moment: I like the idea of using WASM for pure computation (ie functions without side effects and no state outside its bounds) were possible especially with ssi-ssd (people can correct me if I am wrong). I know you could build your whole app and state in wasm, but I don't think that is needed here is it?

Copy link
Contributor

@andorsk andorsk Jan 10, 2023

Choose a reason for hiding this comment

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

@michaelneale fair points. I get using WASM for pure computation, and think a stateless app is probably fair. One question though here: moving keys back and forth between local storage to the WASM application. I don't have an answer here, but does shuffling the key around between WASM and JS runtime render any security issues?

i.e

var pk = JSON.parse(CreatePK()) // generates PK. Now key is JS Object. You could store this somewhere after.
doX(JSON.stringify(pk)) // PK is serialized and sent. 

Totally possible I'm off here.

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

few small comments - awesome to get this going

@decentralgabe
Copy link
Member

Screenshot 2022-12-09 at 10 10 13 AM
two vulns

* This is the glue to bind the functions into javascript so they can be called
*/
func main() {
done := make(chan struct{})
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, you need to keep the process alive.

// 2. Calling a ssi-sdk function directly - but returning a plain old string
func generateKey(_ js.Value, args []js.Value) interface{} {

keyType := args[0].String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth checking the length of arguments?

Copy link
Member

@mistermoe mistermoe Dec 15, 2022

Choose a reason for hiding this comment

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

for sure @andresuribe87 this method will currently blow up if no args are provided. the most i'd do in this PR though is add a TODO above this line saying that arg length needs to be checked and handled appropriate (aka throw error if no args provided). Then, a general pattern can be figured out and applied for error handling in another PR

Copy link
Contributor

@andorsk andorsk Jan 7, 2023

Choose a reason for hiding this comment

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

agreed here. Here was how I did it, but I definitely would want to revisit it. I didn't feel it was robust enough:

// checks the args length with the input
// TODO: more robust argument checking. i.e Maybe align with a validator?
func checkArgs(actual []js.Value, args ...string) error {
	if len(actual) < len(args) {
		return errors.New(fmt.Sprintf("not enough arguments. Need %v", args)) // nit: change to errorf
	}
	return nil
}

then call it later:

err := checkArgs(args, "id")
if err != nil {
   return err.Error()
}

re: blow up: if this blows up, it doesn't recover. Which is a big issue.

So I suggest we have a recover mechanic in place before getting too deep into actually building out the methods.

Check out https://go.dev/blog/defer-panic-and-recover, specifically the recover mechanic native to golang and https://www.geeksforgeeks.org/recover-in-golang/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andorsk wouldn't the main function trap this and restart?

Copy link
Contributor

@andorsk andorsk Jan 10, 2023

Choose a reason for hiding this comment

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

if panic, it would exit main function and it wouldn't restart unless you handled recover. At least, my experience.

wasm/main.go Outdated Show resolved Hide resolved

<head>
<meta charset="utf-8" />
<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.13.1/ace.js" integrity="sha512-IQmiIneKUJhTJElpHOlsrb3jpF7r54AzhCTi7BTDLiBVg0f7mrEqWVCmOeoqKv5hDdyf3rbbxBUgYf4u3O/QcQ==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this is needed?

Copy link
Member

Choose a reason for hiding this comment

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

@andresuribe87 it was just used to make the did doc json output look fancy. can def remove it

@andorsk
Copy link
Contributor

andorsk commented Dec 13, 2022

@michaelneale i've done some work on this actually on my side, but never merged it/sent a PR. Would be happy to sync about it and show some of the stuff I did if that helps.

@michaelneale
Copy link
Contributor Author

@andorsk sure - could open it in a branch if you like or a gist?

@andorsk
Copy link
Contributor

andorsk commented Dec 13, 2022

@michaelneale yea...i need to do some merging with/cleanup to make it work directly with the ssi-sdk ( it's over benri's sdk, but yea..I can either send a PR to this or a new PR. Not sure if GIST makes sense as there is a few files.

Here's an example of a VC I generated using WASM + ssk-sdk

image

Couple things I recommend paying attention to:

  1. I ended up namespacing my commands.
  2. For mage, I ended up abstracting some of the commands to work a little better with the multiple combos that exist.
  3. There's one major issue which I need to investigate:
    • Restarting the connection if it fails and keeping a connection open. It's probably some singleton like format. I tried some polling on my side, but it seems to keep loading the artifact entirely each poll so something is wrong there.

@michaelneale
Copy link
Contributor Author

interesting - I did suspect error handling would need a lot of work.

@mistermoe
Copy link
Member

mistermoe commented Dec 15, 2022

@andorsk +1 to namespacing commands

did some digging around about how to handle error handling. it's unfortunately weird. found this stackoverflow post awhile back which sorta highlights how error handling is weird and lays out 2 approaches (1 of which is kinda yikes)

@mistermoe
Copy link
Member

eventually, might be nice to have a test suite for the bindings. can use any js testing framework and karma for running tests in a headless browser

@andorsk
Copy link
Contributor

andorsk commented Dec 15, 2022

@mistermoe a couple things here:

to your point, some of the issues with error handling on WASM is the following:

  1. As you mentioned, its hard to throw an error
  2. When you start the service, if the service panics for some it crashes the service. And unless you build a mechanic to catch the panic (i.e recover) your service dies.
  3. re: some test suite, yes, but I will say I've done it on my side, and I thought I had it working but it's currently broken ( some issue with build tags ), so now I'm not so sure. It's also annoying b/c you have to keep converting things to js.Value.

@michaelneale
Copy link
Contributor Author

thanks for comments. FYI the main.go is mainly as a first example to get things going, but I guess we need to formalise how to expose these apis with the examples there.

@michaelneale
Copy link
Contributor Author

@andorsk are you happy for me to continue plugging along on this to get wasm in SSI-SDK and then we can iterate on it? or we need a different approach?

@andorsk
Copy link
Contributor

andorsk commented Dec 21, 2022

@michaelneale got for it! I think continuing to plug away on it is important, but my suggestion here is: let's get two functions with a dependency across them really stable ( i say two so we can also consider serialization between functions ). With the mechanics to do it and a way to move from 1 -> N functions easily. Then we can build the rest of the functions out. "stable" means doesn't panic, has robust handling for issues, and has a test suite.

i.e create PPK -> create DID from it over WASM. My implementation required serializing things. It would be great to pipe things together a little better.

Here's what I ended up doing on my side.

  1. Namespaced functions
  2. Serialized outputs using JSON stringify and JSON parse. Sometimes base64 encoding them before sending over.
  3. Added some simple handlers to support error checking ( i.e # of args, etc ).
  4. Wanted to add some panic handling and recovery mechanics. Never did that.
  5. I had some tests, that was working for sometime but something happened with my build tags, and it stopped working. I need to revisit this.

@michaelneale
Copy link
Contributor Author

@andorsk great thoughts - agree- will keep plugging away with hopefully something simple end to end that can be added to.
I am a little confused about namespacing functions thought, is that to avoid polluting the global namespace on the js side?

@andorsk
Copy link
Contributor

andorsk commented Dec 23, 2022

exactly. js scoping is weird. There might be a better way to do it. ( i.e a WASM object ) or something, but I didn't want to run into naming conflicts as the project grew.

michaelneale and others added 6 commits January 6, 2023 18:19
Co-authored-by: Gabe <7622243+decentralgabe@users.noreply.github.com>
Co-authored-by: Gabe <7622243+decentralgabe@users.noreply.github.com>
@TBD54566975 TBD54566975 deleted a comment from decentralgabe Jan 6, 2023
@michaelneale
Copy link
Contributor Author

hey @andorsk tidied some things up, added in a did resolver to try out:

console.log(resolveDid("did:peer:0z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"));

And some retry logic and more (still no tests or good error handling).

Also, and I am ashamed to say this, that function was written mostly by AI.

@@ -73,3 +74,30 @@ func makeDid(_ js.Value, args []js.Value) interface{} {
return js.ValueOf(resultObj)

}

func resolveDid(_ js.Value, args []js.Value) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a way to distinguish in the JS if this was an error vs the return object?

Copy link
Contributor

@andorsk andorsk Jan 7, 2023

Choose a reason for hiding this comment

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

that's a great question. I've had a similar question and I've not been able to determine a way aside from serializing a string with the word "error". Maybe the right way is to build a wrapper around the objects such as the following:

STRAWMAN:

type ObjectType string

cont (
    TypeError ObjectType = "error"
     TypeObject ObjectType = "object"
) 
type ObjectWrapper  {
     ObjectType ObjectType `json:"type"`
     Object interface{} `json:"object"`
}

or something. There's no "standard" way though, AFAIK. The above feels like a hack, but might be the best way to move forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andorsk @nitro-neal I am thinking of a convention based json object being returned with an option error top level key

Copy link
Member

@mistermoe mistermoe Jan 10, 2023

Choose a reason for hiding this comment

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

@nitro-neal @andorsk @michaelneale yeah there's no easy way to throw a JS exception from web assembly (or at least none that i could think of) to support something like

try {
  resolveDid("did:janky:alice")
} catch (e) {
  console.log(error);
}

WASM execution happens within an isolated sandbox in a separate execution environment that cannot directly access the JS stack. So anything thrown from WASM gets caught by the WASM runtime and handled as if the WASM code panicked.

@michaelneale your top level error property could definitely work. Another option that may feel a bit more like catching an exception in JS land would be to return a Promise from WASM that is resolved or rejected. Using await semantics you'd be able to try/catch the returned promise, e.g.

try {
  await resolveDid("did:janky:alice")
} catch (e) {
  console.log(e);
}

the underlying Go code would look like this:

func resolveDid(_ js.Value, args []js.Value) interface{} {
    handler := js.FuncOf(func(this js.Value, args []js.Value) interface{} {
        resolve := args[0]
        reject := args[1]
    
        go func() {
            didString := args[0].String()
            resolvers := []did.Resolution{did.KeyResolver{}, did.WebResolver{}, did.PKHResolver{}, did.PeerResolver{}}
            resolver, err := did.NewResolver(resolvers...)
            
            if err != nil {
                // err should be an instance of `error`, eg `errors.New("some error")`
                errorConstructor := js.Global().Get("Error")
                errorObject := errorConstructor.New(err.Error())
                reject.Invoke(errorObject)
            }
    
            doc, err := resolver.Resolve(didString)
            if err != nil {
                errorConstructor := js.Global().Get("Error")
                errorObject := errorConstructor.New(err.Error())
                reject.Invoke(errorObject)
            }
    
            resultBytes, err := json.Marshal(doc)
            if err != nil {
                errorConstructor := js.Global().Get("Error")
                errorObject := errorConstructor.New(err.Error())
                reject.Invoke(errorObject)
            }
    
            var resultObj map[string]interface{}
            err = json.Unmarshal(resultBytes, &resultObj)
            if err != nil {
                errorConstructor := js.Global().Get("Error")
                errorObject := errorConstructor.New(err.Error())
                reject.Invoke(errorObject)
            }
    
            resolve.Invoke(js.ValueOf(resultObj))
        }()
    
        return nil
    })
    
    promiseConstructor := js.Global().Get("Promise")
    return promiseConstructor.New(handler)
}

the go is def a bit uglier but i bet we can teach ChatGPT to return functions with this kind of wrapper.

random unnecessary info:
the "uneasy" way to throw exceptions from WASM into the JS runtime i think would require us to provide the assembler with a hint. so something like:

// throw stub in our go code
func Throw(exception string, message string)
;; assembler hint in ssi-sdk_js.s

TEXT ·Throw(SB), NOSPLIT, $0
  CallImport
  RET
// manually added to wasm_exec.js into `importObject.go` object
// this object already exists in `wasm_exec.js`, just adding here for sake of example
this.importObject = {
// `go` already exists in `wasm_exec.js`. just adding here for sake of example
  go: {
    // this is what we would need to add ourselves
    // func Throw(exception string, message string)
    'ssi-sdk.Throw': (sp) => {
      const exception = loadString(sp + 8)
      const message = loadString(sp + 24)
      const throwable = globalThis[exception](message)
      throw throwable
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Heavy +1 to returning a promise. DID resolution will typically involve making a network request, which reinforces the fact that it should be an awaitable.

In fact, I can see the Resolve method evolving to become more go idiomatic and accepting a context.Context object as the first parameter. This object would carry cancellation signals.

Copy link
Contributor

@andorsk andorsk left a comment

Choose a reason for hiding this comment

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

My initial thoughts here are:

It's a great starting point! But we need to make sure to iron out the "handling" more before going onto actually building out useful things. Such as:

  1. Error Handling
  2. Recovery
  3. Signals to the service
  4. Argument handling
  5. Namespacing.
  6. Serialization and APIs of keys and other secure things.
  7. Test suite
  8. State handling

// 2. Calling a ssi-sdk function directly - but returning a plain old string
func generateKey(_ js.Value, args []js.Value) interface{} {

keyType := args[0].String()
Copy link
Contributor

@andorsk andorsk Jan 7, 2023

Choose a reason for hiding this comment

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

agreed here. Here was how I did it, but I definitely would want to revisit it. I didn't feel it was robust enough:

// checks the args length with the input
// TODO: more robust argument checking. i.e Maybe align with a validator?
func checkArgs(actual []js.Value, args ...string) error {
	if len(actual) < len(args) {
		return errors.New(fmt.Sprintf("not enough arguments. Need %v", args)) // nit: change to errorf
	}
	return nil
}

then call it later:

err := checkArgs(args, "id")
if err != nil {
   return err.Error()
}

re: blow up: if this blows up, it doesn't recover. Which is a big issue.

So I suggest we have a recover mechanic in place before getting too deep into actually building out the methods.

Check out https://go.dev/blog/defer-panic-and-recover, specifically the recover mechanic native to golang and https://www.geeksforgeeks.org/recover-in-golang/

// 3. Returning a richer object, converting to json and then unmarshalling to make it a js object
func makeDid(_ js.Value, args []js.Value) interface{} {

pubKey, _, _ := crypto.GenerateKeyByKeyType(crypto.Ed25519)
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to think about with WASM is how it interfaces with storage. It's not quite clear cut. I.e you generate the public key in WASM, but you'll now how to figure out a way to transfer and serialize the key to the application handling the storage. That transfer always felt a little "iffy" to me. I.e you've now sent your private key to your application, and that may be a security risk.

@andorsk
Copy link
Contributor

andorsk commented Jan 10, 2023

@michaelneale Thanks for being patient with some of my comments. Re: some of my comments around better handling, I think it would be reasonable to address them in a future PR.

I know there was a lot there, and it probably makes sense to address them after, but note that they are pretty important to have a plan around before expanding features.

This work was really great though! Thanks for putting in the effort to get this into a reality.

@michaelneale
Copy link
Contributor Author

@andorsk no worries at all - has been most helpful!

@andresuribe87 @nitro-neal do you want to take this one over? can merge it as is, or add in Moe's changes for error handling? (either as part of this or a future PR) so that we can expose things as needed?

@andresuribe87
Copy link
Contributor

I can take over! I would suggest merging as is and then fixing as we go.

@michaelneale michaelneale merged commit 5743e2e into main Jan 11, 2023
@michaelneale michaelneale deleted the wasm branch January 11, 2023 02:44
@michaelneale
Copy link
Contributor Author

done!

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

6 participants