feat: add zklogin verify endpoint to jsonrpc #21166
Conversation
joyqvq
requested review from
amnn,
wlmyng,
stefan-mysten and
emmazzz
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
joyqvq
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
joyqvq
force-pushed
the
joy/add-zklogin-to-rpc
branch
from
35deb99 to
07c494f
Compare
joyqvq
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
joyqvq
changed the title
joyqvq
force-pushed
the
joy/add-zklogin-to-rpc
branch
from
07c494f to
345faf7
Compare
joyqvq
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| _intent_scope: ZkLoginIntentScope, | ||
| _author: SuiAddress, | ||
| ) -> RpcResult<ZkLoginVerifyResult> { | ||
| todo!() |
There was a problem hiding this comment.
confirming i do not need to add this to indexer api?
There was a problem hiding this comment.
Correct, fine to leave this as we don't use this in production anymore -- but let's have it return an error rather than panic.
|
|
||
| /// Verify a zklogin signature | ||
| #[method(name = "verifyZkLoginSignature")] | ||
| async fn verify_zklogin_signature( |
There was a problem hiding this comment.
nit: could you also document what the parameters are? You can probably lift the docs from what you wrote for GraphQL. The only difference is that here you add the doc comments directly on the parameters.
There was a problem hiding this comment.
agreed that providing better docs for what each of the parameters is expected to be would be nice.
There was a problem hiding this comment.
The change looks good to me, but I'm less familiar with how things should be done on a fullnode (e.g. is it okay to fetch the per-epoch store inside an RPC method implementation, is there a better way to get the current epoch, etc), so I'll leave it with someone more familiar with that part to review it, but everything else LGTM!
| let mut oidc_provider_jwks = ImHashMap::new(); | ||
| for active_jwk in new_jwks.iter() { | ||
| let ActiveJwk { jwk_id, jwk, .. } = active_jwk; | ||
| match oidc_provider_jwks.entry(jwk_id.clone()) { | ||
| im::hashmap::Entry::Occupied(_) => { | ||
| warn!("JWK with kid {:?} already exists", jwk_id); | ||
| } | ||
| im::hashmap::Entry::Vacant(entry) => { | ||
| entry.insert(jwk.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think its a bit of a shame that we need to load this every time but 🤷🏼 i'm sure its not that big of a deal.
joyqvq
force-pushed
the
joy/add-zklogin-to-rpc
branch
from
345faf7 to
dc8b36d
Compare
joyqvq
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
* 'main' of https://github.com/MystenLabs/sui: (84 commits) [Consensus] enable the new linearize logic for devnet. (MystenLabs#21172) Minor change to indicate that GraphQL is at alpha, not at beta (MystenLabs#21169) feat: add zklogin verify endpoint to jsonrpc (MystenLabs#21166) [sui-adapter] Remove error traces from native functions (MystenLabs#21185) [consensus] Enable zstd compression for consensus tonic network in mainnet (MystenLabs#21177) Reduce thread stall log level from ERROR to WARN (MystenLabs#21175) proto: sort fds files for stable codegen rpc: impl Name for google.rpc types rpc: properly return error code rpc: remove reqwest dependency grpc: add protos for more well known types proto: rework directory structure to conform with standards Update publish.mdx (MystenLabs#21170) Update ptb.mdx (MystenLabs#21171) [exp-to-insta][14/14] Cleanup of insta_assert macro (MystenLabs#21114) fix(sui): fix Option<address> serialization in client_ptb (MystenLabs#21126) [exp-to-insta][13/n] Cleanup insta migration (MystenLabs#21113) [exp-to-insta][12/n] Migrate move-analyzer (MystenLabs#21112) [exp-to-insta][11/n] Migrate move-stackless-bytecode and move-model (MystenLabs#21111) [exp-to-insta][10/n] Migrate move-unit-test (MystenLabs#21109) ...
Description
add zklogin verify to jsonrpc. this is a mirror of whats in graphql already: #16625
Test plan
simtest
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.