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: add portal to aa-signers #303

Merged
merged 7 commits into from
Dec 12, 2023
Merged

Conversation

avasisht23
Copy link
Contributor

@avasisht23 avasisht23 commented Dec 4, 2023

Pull Request Checklist


PR-Codex overview

Detailed summary

  • Added PortalSigner to the @alchemy/aa-signers package.
  • Updated packages/signers/package.json to include @portal-hq/web as a dependency.
  • Added PortalAuthenticationParams and PortalUserInfo interfaces to packages/signers/src/portal/types.ts.
  • Updated templates/typescript/base.json to include "dom" in the lib array and "es2021" as the target in the compilerOptions.
  • Updated site/.vitepress/config.ts to include a new section for Portal Signer in the navigation menu.
  • Added new Markdown files for getAddress, signMessage, getAuthDetails, and authenticate methods of PortalSigner in site/packages/aa-signers/portal directory.
  • Updated site/packages/aa-signers/portal/guides/portal.md with installation instructions for @portal-hq/web SDK and updated code snippets.
  • Added PortalSigner class to packages/signers/src/portal/signer.ts with methods for getAddress, signMessage, signTypedData, and authenticate.

The following files were skipped due to too many changes: packages/signers/src/portal/signer.ts, site/packages/aa-signers/portal/signTypedData.md, site/packages/aa-signers/portal/introduction.md, site/packages/aa-signers/portal/constructor.md, packages/signers/src/portal/__tests__/signer.test.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@avasisht23
Copy link
Contributor Author

avasisht23 commented Dec 4, 2023

@avasisht23
Copy link
Contributor Author

avasisht23 commented Dec 4, 2023

@moldy530 @denniswon see this post as to why this fails CI checks.

tldr; portal SDK doesn't export things properly. I've asked them to revise. When I go into node_modules myself and edit, build and tests pass.

@avasisht23 avasisht23 force-pushed the 12-03-feat_add_fireblocks_to_aa-signers branch from 8ee5ca3 to dee199a Compare December 4, 2023 19:42
@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from 33bc19c to ce64f04 Compare December 4, 2023 19:42
@avasisht23 avasisht23 mentioned this pull request Dec 4, 2023
7 tasks
@avasisht23 avasisht23 force-pushed the 12-03-feat_add_fireblocks_to_aa-signers branch from dee199a to ccdca68 Compare December 4, 2023 19:44
@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from ce64f04 to d072eb3 Compare December 4, 2023 19:44
@avasisht23 avasisht23 force-pushed the 12-03-feat_add_fireblocks_to_aa-signers branch from ccdca68 to e55a747 Compare December 4, 2023 21:57
@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from d072eb3 to 1b2c722 Compare December 4, 2023 21:57
@avasisht23 avasisht23 mentioned this pull request Dec 5, 2023
7 tasks
autoApprove: true,
gatewayConfig: `${polygonMumbai.rpcUrls.alchemy.http}/${process.env.ALCHEMY_API_KEY}`,
chainId: 80001,
host: process.env.PORTAL_WEB_HOST,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this not necessary anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope it was optional on their constructor params, but fair can add it back if necessary

@@ -2,7 +2,7 @@
"$schema": "https://json.schemastore.org/tsconfig",
"display": "Default",
"compilerOptions": {
"lib": ["es2022"],
"lib": ["es2022", "dom"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I forgot to send the screenshot, but portal has a bunch of window/dom-related elements in their SDK. I had to add this to build, other was "window not defined" related errors when trying to build.

Choose a reason for hiding this comment

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

We'll update our docs to include that the Portal SDK requires this!

@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from 1b2c722 to f9ab1a3 Compare December 7, 2023 22:18
@avasisht23 avasisht23 force-pushed the 12-03-feat_add_fireblocks_to_aa-signers branch from f369f33 to 6f369aa Compare December 11, 2023 18:31
@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from f9ab1a3 to 6352450 Compare December 11, 2023 18:31

export interface PortalAuthenticationParams {}

// taken from Portal SDK since not exported

Choose a reason for hiding this comment

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

We can export this for y'all!

Choose a reason for hiding this comment

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

We'll queue up an update.

@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from 6352450 to 92e0783 Compare December 11, 2023 19:54
Base automatically changed from 12-03-feat_add_fireblocks_to_aa-signers to main December 11, 2023 20:04
@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from 92e0783 to a222990 Compare December 11, 2023 20:08
@avasisht23 avasisht23 force-pushed the 12-04-feat_add_portal_to_aa-signers branch from f62b803 to 926fd84 Compare December 11, 2023 20:38
Copy link

@dscrobonia dscrobonia left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

@moldy530 moldy530 left a comment

Choose a reason for hiding this comment

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

had small nit, but LGTM otherwise

};

authenticate = async () => {
this.signer = new WalletClientSigner(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just create this in the constructor and have authenticate return this.getAuthDetails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just for convention, because we want the dev to call authenticate before calling getAddress, sendMessage, etc. like is necessary with the other signers

@avasisht23 avasisht23 merged commit eb8a0c3 into main Dec 12, 2023
4 checks passed
@avasisht23 avasisht23 deleted the 12-04-feat_add_portal_to_aa-signers branch December 12, 2023 00:55
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.

None yet

3 participants