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(passport): Revocation of smart contract wallet scope #2236

Merged
merged 15 commits into from
Jun 19, 2023

Conversation

poolsar42
Copy link
Collaborator

@poolsar42 poolsar42 commented May 4, 2023

Description

Revoke Smart contract wallet session key

Updates docs to indicate revocation of SC wallet session keys

Related Issues

Testing

  1. Authorise app for erc_4337 scope:
    localhost:10001/authorize?client_id=b84308327fccec7d52d454011286c6d8&redirect_uri=http://localhost&state=foo&scope=erc_4337&prompt=consent
  2. After it's done, query galaxy to create session key for this SC wallet:
    curl http://localhost:10401/graphql \ -X POST \ -H 'content-type: application/json' \ -H 'X-GALAXY-KEY: %GALAXY APY KEY%' \ -H 'Authorization: Bearer %ACCESS TOKEN%' \ --data '{"query":"mutation { registerSessionKey (sessionPublicKey: \"%DEVELOPER'S WALLET ADDRESS%\", smartContractWalletAddress: \"%SMART WALLET BLOCKCHAIN ADDRESS%\" )}"}'

This endpoint will return session key, test then this session key - e.g. mint NFT with this. Here' my script

  1. Revoke Access for authorised app
  2. Try minting NFT with same session key once again

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@poolsar42 poolsar42 self-assigned this May 4, 2023
@poolsar42 poolsar42 force-pushed the feat/2119-passport-revoke-SC-wallet-scope branch 3 times, most recently from 08f1d59 to 61294b6 Compare May 12, 2023 18:17
@poolsar42 poolsar42 marked this pull request as ready for review May 12, 2023 22:07
@poolsar42 poolsar42 marked this pull request as draft May 15, 2023 13:02
@poolsar42 poolsar42 force-pushed the feat/2119-passport-revoke-SC-wallet-scope branch 2 times, most recently from b264d9a to 5b06d12 Compare May 16, 2023 16:38
@poolsar42 poolsar42 force-pushed the feat/2119-passport-revoke-SC-wallet-scope branch 2 times, most recently from bd24657 to ce51895 Compare June 14, 2023 16:37
@@ -1,4 +1,4 @@
import { useFetcher } from '@remix-run/react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only valuable change in this file - I change fetcher.Form on Form from @remix-run/react. Other changes here are from prettier.

@@ -43,6 +43,7 @@ interface CreateInnerContextOptions
DELAY_BETWEEN_REGENERATION_ATTEMPTS_IN_MS: number
REGENERATION_COOLDOWN_PERIOD_IN_MS: number
MAX_ATTEMPTS_TIME_PERIOD_IN_MS: number
MUMBAI_PROVIDER_URL: string
Copy link
Collaborator Author

@poolsar42 poolsar42 Jun 14, 2023

Choose a reason for hiding this comment

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

Keep it as Mumbai for now - will switch to ethereum mainnet after we decide how to do this properly

Copy link
Contributor

Choose a reason for hiding this comment

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

should be based on project id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be based on the env I'd say and project id should be based on env as well (I think this is how it is now, right?)

Copy link
Contributor

Choose a reason for hiding this comment

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

what env? project id's are tied to the chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am talking about our env - for everything not in prod it should probably be tied to Mumbai and only mainnet on prod

@@ -25,6 +25,7 @@ export interface Environment {
DELAY_BETWEEN_REGENERATION_ATTEMPTS_IN_MS: number
REGENERATION_COOLDOWN_PERIOD_IN_MS: number
MAX_ATTEMPTS_TIME_PERIOD_IN_MS: number
MUMBAI_PROVIDER_URL: string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same about Mumbai in different file

@poolsar42 poolsar42 marked this pull request as ready for review June 14, 2023 17:01
@poolsar42 poolsar42 changed the title feat(address): revoke session key sc_wallet feat(address): revoke session key for SC wallets Jun 14, 2023
@poolsar42 poolsar42 changed the title feat(address): revoke session key for SC wallets feat(passport): Revocation of smart contract wallet scope Jun 14, 2023
@poolsar42 poolsar42 force-pushed the feat/2119-passport-revoke-SC-wallet-scope branch from 287b8a9 to 9bdfb4f Compare June 14, 2023 18:50
@@ -43,6 +43,7 @@ interface CreateInnerContextOptions
DELAY_BETWEEN_REGENERATION_ATTEMPTS_IN_MS: number
REGENERATION_COOLDOWN_PERIOD_IN_MS: number
MAX_ATTEMPTS_TIME_PERIOD_IN_MS: number
MUMBAI_PROVIDER_URL: string
Copy link
Contributor

Choose a reason for hiding this comment

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

should be based on project id?

clientId,
})

if (appData?.smartWalletSessionKeys?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this object should have the project id that was used to register the session key? It should also check if the session key is expired or not so we don't have to revoke it?

Copy link
Contributor

Choose a reason for hiding this comment

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

to be more generic, it should state the provider and the the provider secret used to register it.

e.g. provider: ZeroDev, config: {projectId: <project id>}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This revocation happens only for a specific app. In our current schema we have getPaymaster endpoint in starbase which returns exactly this generic information about paymaster provider you mentioned in the last comment.

export const PaymasterProviderSchema = z.literal('zerodev')

export const PaymasterSchema = z
  .object({
    provider: PaymasterProviderSchema,
    secret: z.string(),
  })
  .optional()

as for appData it stores smart wallet addresses and dev's address to which session key was issued. Since per one app we have one session key there's no point of storing it additionally in appData.

@poolsar42 poolsar42 force-pushed the feat/2119-passport-revoke-SC-wallet-scope branch from 6174b4c to 30a824b Compare June 19, 2023 16:35
@maurerbot maurerbot merged commit c3447f8 into main Jun 19, 2023
12 checks passed
@maurerbot maurerbot deleted the feat/2119-passport-revoke-SC-wallet-scope branch June 19, 2023 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants