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

feat(rpc): Implement WASM Client #210

Merged
merged 5 commits into from
Jun 14, 2024
Merged

Conversation

distractedm1nd
Copy link
Contributor

This PR adds a WASM-compatible Client implementation. I am fairly new to Rust, so apologies if there is an easier way to get around the trait implementations.

jsonrpsee-wasm-client only supports Websocket, and also does not have the ability to set headers. This means that celestia-node needs to run with --rpc.skip-auth, which is coming out in the next release.

Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

This looks nice.
For the wasm test to properly execute we'd need to add the mentioned flag here.
One thing I'm wondering about is that when it's released and the flag is added, we will no longer need tokens in other tests, but also then we'll have no way to test with token

rpc/Cargo.toml Outdated Show resolved Hide resolved
rpc/tests/wasm.rs Show resolved Hide resolved
@zvolin
Copy link
Member

zvolin commented Feb 1, 2024

we could run 2 bridge nodes and only one with --rpc.skip-auth. I'm not sure if it's worth it tho and if no other tests asserting something that would break with 2 nodes, like peers they discover or sth

rpc/src/client.rs Outdated Show resolved Hide resolved
rpc/src/client.rs Outdated Show resolved Hide resolved
@distractedm1nd
Copy link
Contributor Author

Hey, I had to take an unexpected break for 2 months but I am back. I have implemented the review changes AFAIU, so I think this should be ready to go soon.

The one thing that I'm unsure about is that now run-bridge uses skip-auth, so the auth mechanism is not actually being tested. If there are tests for the negative case I'm assuming CI will fail on this PR

@zvolin zvolin requested review from fl0rek and oblique April 16, 2024 08:03
Copy link
Member

@fl0rek fl0rek left a comment

Choose a reason for hiding this comment

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

Thanks for helping us out!

We still might want to test with auth enabled, but as you've noted we aren't covering negative auth cases now, so that's not a hard requirement and we can fix that later by ourselves.

I think once CI is running and green we can get this merged 🎸

ci/run-bridge.sh Outdated Show resolved Hide resolved
rpc/tests/wasm.rs Show resolved Hide resolved
@distractedm1nd distractedm1nd force-pushed the wasm-client branch 3 times, most recently from 2dc94f3 to b1ec907 Compare June 10, 2024 13:34
Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

loooks good

@zvolin zvolin requested a review from fl0rek June 10, 2024 14:31
Copy link
Member

@fl0rek fl0rek left a comment

Choose a reason for hiding this comment

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

LGTM!

@zvolin
Copy link
Member

zvolin commented Jun 11, 2024

Hey @distractedm1nd, all looks good but the merge is blocked due to the unsigned commits. Would you mind signing them with your gpg key and force pushing?

rpc/Cargo.toml Outdated Show resolved Hide resolved
feat: wasm client

fix: updating run-bridge to use skip auth, organizing imports

fix(ci): adding wasm-pack for rpc crate

fixing workflow to run wasm test as integration test

fix: using wasm-bindgen feature flag
@distractedm1nd
Copy link
Contributor Author

@zvolin I've squashed all the commits into a single, signed commit. Hope this is fine

rpc/src/lib.rs Show resolved Hide resolved
rpc/Cargo.toml Outdated Show resolved Hide resolved
rpc/Cargo.toml Outdated Show resolved Hide resolved
rpc/Cargo.toml Outdated Show resolved Hide resolved
oblique and others added 4 commits June 14, 2024 11:39
Signed-off-by: Yiannis Marangos <psyberbits@gmail.com>
Signed-off-by: Yiannis Marangos <psyberbits@gmail.com>
@oblique oblique changed the title feat: WASM Client feat(rpc): Implement WASM Client Jun 14, 2024
@oblique oblique enabled auto-merge (squash) June 14, 2024 08:58
@oblique oblique merged commit 163cefb into eigerco:main Jun 14, 2024
6 checks passed
@oblique
Copy link
Member

oblique commented Jun 14, 2024

Thank @distractedm1nd!

@zvolin zvolin mentioned this pull request Jun 14, 2024
@zvolin zvolin mentioned this pull request Jul 5, 2024
@zvolin zvolin mentioned this pull request Jul 25, 2024
@zvolin zvolin mentioned this pull request Aug 13, 2024
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.

4 participants