-
Notifications
You must be signed in to change notification settings - Fork 1
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 for user to participate in DKG and signing protocols #414
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Does that mean that we don't need separate WASM bindings in the Synedrion repo? |
@fjarri that was the idea. but i am starting to think this idea of making JS bindings to a function that involves a websocket client is too ambitious. it works in the browser but i think it is never going to work on all platforms. Unless maybe we use WASI. but either way, we no longer need the |
* master: Write a Dockerfile that can build both `entropy` and `server`. (#430) Workflow for automated GitHub Release drafting/publishing. (#429) Update `.editorconfig` to match `.rustfmt.toml` settings (#427) Place `demo_offence` dispatchable behind root origin check (#426) Ensure correct validator order by using ValidatorInfo from chain rather than from user (#425) changed remaining references of --ws-external to --rpc-external Update README.md proactive refresh (#413)
make build-nodejs | ||
cd nodejs-test | ||
yarn | ||
cd ../../.. |
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.
Im not sure i've done this stuff in CI in the best way, but unless there is something really bad, i'd prefer to make suggest changes to it in a separate PR. Because this branch is constantly having conflicts and i want to get it merged.
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.
This looks cool! I'm still trying to figure out the context around this, so I haven't able to do a deep code review yet.
Okay so from what I can tell this PR is a couple steps towards #362, right?
And, correct me if I'm wrong here, but what it does is:
- it enables parts of the
server
(just theprotocol
crate?) to be compiled to Wasm, which - allows the server to be bundled in an app with the Entropy SDK, which
- lets a user connect directly to other TSS servers from the brower and participate in DKG themselves.
And how would you recommend to review this PR? Anywhere that should be looked at first or more deeply?
} | ||
|
||
/// Details of a validator intended for use on JS | ||
/// This differs from [crate::ValidatorInfo] only in that the fields must be private |
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.
We can always make the other struct's fields private as well and add a constructor or something to initialize it
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.
Hmm yeah it would be nice to just have one type. Although currently this type is usually created by being deserialized from json in a signature request. Do you think that would stop working if we made the fields private? I wanna avoid making breaking changes where possible.
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 want to say the field visibility shouldn't matter for deserializing, but I'm not 100% sure. We can file this as part of a general type-unification issue and deal with it later
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.
Made #507
Co-authored-by: Hernando Castano <[email protected]>
Yep exactly. The stuff in the
idk, i think your comments are already useful and you understand pretty well whats going on here so just check for red flags |
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.
Some smalls comments and questions but otherwise looks good! I'm happy to get this merged and continue improving on it iteratively.
In practice this has turned out to be quite tricky and i'm not sure it was the best decision.
Would you be able to elaborate a bit more on this? What other approach would you suggest?
] | ||
|
||
[lib] | ||
crate-type=["cdylib", "rlib"] |
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.
Why do we need to specify rlib
here?
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.
Without it, we can't use this crate as a dependency of server
. cdylib
will make it a dynamic library and if we want to also be able to compile it statically we need to make that explicit.
// TODO if this fails, the ws connection has been dropped during the protocol | ||
// we should inform the chain of this. |
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 know it's not part of your PR, but maybe you know.
What would "informing the chain" look like in this case? I'm also wondering if the chain should even care in this case since it's mostly a TSS <-> TSS problem right?
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.
ya but we should be noting like hey one party failed, this is a griefing issue and if a specific validator keeps getting reported for this then should be removed from the validating pool and potentially slashed
#[cfg_attr(feature = "wasm", wasm_bindgen)] | ||
pub async fn run_signing_protocol( | ||
key_share: String, | ||
sig_uid: String, |
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.
Doing this as a follow up sounds fine to me
} | ||
|
||
/// Details of a validator intended for use on JS | ||
/// This differs from [crate::ValidatorInfo] only in that the fields must be private |
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 want to say the field visibility shouldn't matter for deserializing, but I'm not 100% sure. We can file this as part of a general type-unification issue and deal with it later
|
||
/// For testing running the protocol on wasm, spawn a process running nodejs and pass | ||
/// the protocol runnning parameters as JSON as a command line argument | ||
pub async fn spawn_user_participates_in_signing_protocol( |
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.
What do you think about moving some of these tests out to an integration test folder instead? Seems strange to me that unit tests would be spawning a NodeJS process for example.
Although you did say that eventually those should be run as part of the SDK test suite, right?
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.
Made #508
@@ -22,7 +22,7 @@ zeroize ={ version="1.4", features=["zeroize_derive"], default-features= | |||
rpassword ={ version="5.0", default-features=false } | |||
scrypt ={ version="0.11.0", default-features=false, features=["std"] } | |||
chacha20poly1305={ version="0.9", features=["alloc"], default-features=false } | |||
synedrion ={ git="ssh://[email protected]/entropyxyz/synedrion.git", tag="v0.0.9" } | |||
synedrion ={ git="ssh://[email protected]/entropyxyz/synedrion.git", branch="fix-32bit" } |
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.
Personally I don't love going to branches, ok for now to get this in but please make an issue to fix and fix when you can
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.
@@ -173,7 +175,9 @@ pub async fn execute_dkg( | |||
) -> Result<KeyShare<KeyParams>, ProtocolExecutionErr> { | |||
let party_ids: Vec<PartyId> = | |||
threshold_accounts.clone().into_iter().map(PartyId::new).collect(); | |||
// TODO #499 this will panic if we give an out of bounds my_idx |
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.
is this not handled with the bad key share?
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.
Yeah i think you are right, i don't think theres currently any way this function could get called with an bad value for my_idx
@@ -261,6 +265,7 @@ pub async fn execute_proactive_refresh( | |||
) -> Result<KeyShare<KeyParams>, ProtocolExecutionErr> { | |||
let party_ids: Vec<PartyId> = | |||
threshold_accounts.clone().into_iter().map(PartyId::new).collect(); | |||
// TODO #499 this will panic if we give an out of bounds my_idx |
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.
same
// TODO if this fails, the ws connection has been dropped during the protocol | ||
// we should inform the chain of this. |
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.
ya but we should be noting like hey one party failed, this is a griefing issue and if a specific validator keeps getting reported for this then should be removed from the validating pool and potentially slashed
#[cfg_attr(feature = "wasm", wasm_bindgen)] | ||
pub async fn run_signing_protocol( | ||
key_share: String, | ||
sig_uid: String, |
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.
https://github.com/paritytech/ss58-registry/tree/main also I think this lib would work but ya im cool with making an issue and having someone follow up
Co-authored-by: Hernando Castano <[email protected]>
Co-authored-by: Hernando Castano <[email protected]>
The problem is that this contains networking code (websockets), which i expect is going to cause problems on some platforms. I would have liked to have this crate only deal with protocol-level stuff, (handshaking, encryption, and synedrion protocol execution) and leave the networking stuff out of it. But that would mean having a wasm interface with some sort of channels or streams (nodejs streams?) which i don't know how to do. |
This adds wasm bindings to functions needed for client-side participation in the protocols
TODO
gloo-net::websocket
instead oftungstenite
on wasmwasm_bindgen_futures::spawn_local
instead oftokio::spawn
on wasmwasm_bindgen
to functions and make sure the types work