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

Simplify nodejs and wasm bindings p. 2 #1146

Conversation

kwek20
Copy link
Contributor

@kwek20 kwek20 commented Sep 6, 2023

====
This merges into the original to show the difference
Please discuss below ;)

Changes:

  • Wrapped all errors returned to js in èrrorHandler, making parsing on 1 spot
  • Added a lot of try/catch to otherwise methods (and make them call errorHandle. This used to return just the str error, not a serialized object)
  • Made most methods that didnt need async sync. neon does NOT like async.
  • Made wasm side methods sync when possible
  • Removed Unneeded error logging

Questions:

  • Do we just want to return a string again, or return the whole error object (I vote string for now)
  • Alternatively, we can make all methods async, this needs more work on neon side, but makes JS code more maintainable
  • Anyone knows what the heck is up with SecretManager on wasm?? (
    // TODO figure out why this one is extensible but client isnt
    )

Todo:

  • I noticed i missed wallet.listen cannot be sync, need to revert

Heres my beautifull design incase youre wondering how it all stitches together
Screenshot_841

@kwek20 kwek20 changed the title Chore/bindings cleanup is alive 2 Simplify nodejs and wasm bindings p. 2 Sep 6, 2023
const getSecretManagerFromWallet = (
method: WalletMethodHandler,
): SecretManagerMethodHandler => {
// TODO figure out why this one is extensible but client isnt

Choose a reason for hiding this comment

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

TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea i cannot figure this one out unfortunately, should i make a ticket or do we accept this solution? :)

Choose a reason for hiding this comment

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

what is the problem exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SecretManagerMethodHandler constructor checks the type it receives, if its extensible, its a config/serialized info. if not, its a rust object, and its ready for use. However, for some reason the SecretManagerMethodHandler we return in wasm is diffferent from the nodejs one, and is extensible, causing it to crash. (trying to deserialize a rust pointer)
Thats why here i intercept the wasm method and just force "not extensible". it works :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(probably because nodejs returns Arc<>, but we cant do that in wasm as its not implementing some trait (i forgot which, some wasm stuff)

@thibault-martinez thibault-martinez merged commit daa1785 into iotaledger:chore/bindings-cleanup-is-alive Sep 7, 2023
kwek20 pushed a commit to kwek20/iota-sdk that referenced this pull request Oct 30, 2023
* unify error handling and responses

* wasm

* its something

* secret manager alignment

* error on callback fix

* listen error in wasm, madelisten async again
kwek20 pushed a commit that referenced this pull request Dec 13, 2023
* simplify nodejs client and wallet

* got -> was

* fixed client on node side

* wasm

* removed await for client

* lint n format

* fixed test and client error return

* fixed error and comment

* Simplify nodejs and wasm bindings p. 2 (#1146)

* unify error handling and responses

* wasm

* its something

* secret manager alignment

* error on callback fix

* listen error in wasm, madelisten async again

* final changes

* removed unused PromiseString

* lint

* wasm update

* nodejs

* removed async closure

* nodejs fixed with tests

* fmt

* some fixes

* mqtt

* rewrite again eh :D

* client.create()

* format and fmt

* review and 8050

* typo

* updated error handling in nodejs once again

* wasm destroy rename

* Update bindings/nodejs/lib/index.ts

Co-authored-by: DaughterOfMars <[email protected]>

* secret manager create

* typo grrr

* forgot to rename create methods

* Done more comment explaining :D

* removed mqtt custom serde

* wasm fixes

* fix tests

* fmt

* fixed secret

* promise await

* fixed errors, separate eslint

* wasm side

* typo

---------

Co-authored-by: Alex Coats <[email protected]>
Co-authored-by: Thibault Martinez <[email protected]>
Co-authored-by: /alex/ <[email protected]>
Co-authored-by: Thoralf-M <[email protected]>
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.

3 participants