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: integrate zkLogin #7

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

olehmisar
Copy link

@olehmisar olehmisar commented Dec 8, 2024

Uses @shield-labs/zklogin package.

TODO:

  • Backup and recover methods in the solidity contract
  • Backup and recover boilerplate
  • Fix type errors
  • Grab JWT from Google
  • Trustless posting of Google public keys on chain
  • Service to maintain account_id salt to preserve user privacy (currently hardcoded to be 0)
  • Fix experimental_addBackup
  • Implement experimental_recover
  • Implement experimental_removeBackup, experimental_listBackups
  • Use UltraHonk for 10-second proving times (with higher gas cost). Blocked by feat: UltraHonk shield-labs-xyz/zklogin#5

The spec should look something like this:

{
  "method": "experimental_addBackup",
  "params": [{
    "address": "0x...",       // Optional: Defaults to the connected account
    "backupOptions": [{        // Required for "add" and "remove" actions
      "type": "openId" | "email" | "socialRecovery",
      "provider": "google" | "apple" | "email" | "custom",
      "identifier": "[email protected]", // Email or unique identifier
    }]
  }]
}

{
  "method": "experimental_recover",
  "params": [{
    "address": "0x...",
    "backupOption": {
      "type": "openId" | "email" | "socialRecovery",
      "provider": "google" | "apple" | "email" | "custom",
      "proof": "..."       // Proof of ownership
    },
    "newAuthentication": {
      "passkey": "..."     // Details for setting up a new passkey or authentication method
    }
  }]
}

Copy link

changeset-bot bot commented Dec 8, 2024

🦋 Changeset detected

Latest commit: c2847ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
porto Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Dec 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oddworld ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 14, 2024 2:20pm
oddworld-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 14, 2024 2:20pm
porto-wagmi ❌ Failed (Inspect) Dec 14, 2024 2:20pm

Copy link

vercel bot commented Dec 8, 2024

@olehmisar is attempting to deploy a commit to the Ithaca Team on Vercel.

A member of the Team first needs to authorize it.

@jxom
Copy link
Member

jxom commented Dec 14, 2024

It looks like some bundlers are also not happy with usage of pedersen in @aztec/foundation as it also includes node:crypto which doesn't exist in the browser – this is a side-effect of using the Fr class (I don't think this is tracked in AztecProtocol/aztec-packages#8881). I am wondering if your zklogin package could instead perhaps use @paulmillr's pederson hash fn from scure-starknet: https://github.com/paulmillr/scure-starknet/blob/main/index.ts#L253-L258 instead of @aztec/foundation? This may remove all the target & resolution config in vite.config.ts, as well as the Buffer polyfill too.

@olehmisar
Copy link
Author

@jxom i would instead remove @aztec/foundation altogether in favour of @aztec/bb.js. It will still require polyfilling Buffer at the moment but it's much easier to remove Buffer from bb.js than from foundation. I can work with Aztec team on removing Buffer from bb.js

@jxom
Copy link
Member

jxom commented Dec 14, 2024

Sounds great! I am also wondering what we can do to mitigate the massive bundle size jump. It seems that this adds ~890kB to the brotli minified bundle.

$ pnpm check:size

  Package size limit has exceeded by 890.87 kB
  Size limit:   54 kB
  Size:         944.87 kB with all dependencies, minified and brotlied
  Loading time: 18.5 s    on slow 3G
  Running time: 2.6 s     on Snapdragon 410
  Total time:   21 s

Can repro by running pnpm check:size on this branch.

@olehmisar
Copy link
Author

@jxom I removed @aztec/foundation and all polyfills. target: "next" are still required due to AztecProtocol/aztec-packages#10184.

The large size of bundle is due to the size of bb.js wasm. It is loaded lazily using await import. I can consult with Aztec team what can be done to reduce the size.

Copy link
Author

@olehmisar olehmisar left a comment

Choose a reason for hiding this comment

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

notes

@@ -606,6 +608,71 @@ export declare namespace prepareInitialize {
}
}

const zkLogin = new zklogin.ZkLogin()
Copy link
Author

Choose a reason for hiding this comment

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

probably shouldn't be initialized here

Comment on lines +192 to +193
// TODO: import from `@shield-labs/zklogin`
type ZkLoginProvider = 'google' // | 'apple'
Copy link
Author

Choose a reason for hiding this comment

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

fix this todo

@gakonst
Copy link

gakonst commented Dec 16, 2024

What should we do about the published certs? I don't want the registry to be onlyOwner nor multisig/dao, would strongly want immutable. Maybe we have versions in the registry and users can opt-in to a rotation?

@olehmisar
Copy link
Author

@gakonst certs are rotated regularly, so we can’t make them immutable. We can probably leverage a blind oracle (aka TLSNotary) to publish certs. It’s an MPC network running inside a bunch of TEEs that do not see what they signs. It requires Porto to depend on an oracle service provider. vlayer.xyz provides this kind of service.

@gakonst
Copy link

gakonst commented Dec 17, 2024

not comfortable w/ introducing such 3rd party deps...will think about it. maybe we run this ourselves.

@olehmisar
Copy link
Author

the choice boils down to a multisig oracle.

TLDR: MPC with no secret data = a multisig. A blind oracle that posts exclusively Google/Apple certs = a multisig.

Why other options will not work:

  1. MPC does not make sense because there is no secret data, so MPC would be a pure overhead with no advantage over a multisig
  2. Blind oracles (aka TLSNotary, aka zkTLS, aka WebProofs) - similar to the option 1. If we using a blind oracle to post ONLY google/apple certs, then we don't benefit from the blindness property. A blind oracle is only useful when it posts different kinds of data. It's similar to an anonymity set in a privacy pool - the larger the anonymity set the more private the pool is. In blind oracles, the more types of data the oracle posts, the harder it is for the oracle to tamper with data because it won't know what it is tampering with: a Google cert or e.g. an Uber proof of driver.

@gakonst
Copy link

gakonst commented Dec 20, 2024

I can consult with Aztec team what can be done to reduce the size.

Circling back... @olehmisar did u get any news on this? That'd be epic if we could get done. I'm talking with the CF team about potentially doing SXG for the certs, circling back soon.

@olehmisar
Copy link
Author

@gakonst yes, we can reduce the size at least by a factor of x2. And if that’s not enough, we can then bundle an application-specific wasm that only includes functions needed for zklogin to reduce size even more

@gakonst
Copy link

gakonst commented Jan 5, 2025

Also worth looking into https://github.com/zkemail/jwt-tx-builder

@olehmisar
Copy link
Author

They are similar. ZkLogin has a nicer API imo as I tried to keep it as simple as possible. Are you missing anything specific in zkLogin from jwt-tx-builder?

Also, @gakonst are there any updates from cloudflare regarding jwt public keys over SXG?

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