-
Notifications
You must be signed in to change notification settings - Fork 113
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
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
@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. |
8ee5ca3
to
dee199a
Compare
33bc19c
to
ce64f04
Compare
dee199a
to
ccdca68
Compare
ce64f04
to
d072eb3
Compare
ccdca68
to
e55a747
Compare
d072eb3
to
1b2c722
Compare
autoApprove: true, | ||
gatewayConfig: `${polygonMumbai.rpcUrls.alchemy.http}/${process.env.ALCHEMY_API_KEY}`, | ||
chainId: 80001, | ||
host: process.env.PORTAL_WEB_HOST, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
1b2c722
to
f9ab1a3
Compare
f369f33
to
6f369aa
Compare
f9ab1a3
to
6352450
Compare
|
||
export interface PortalAuthenticationParams {} | ||
|
||
// taken from Portal SDK since not exported |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
6352450
to
92e0783
Compare
92e0783
to
a222990
Compare
f62b803
to
926fd84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this 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( |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
Pull Request Checklist
yarn test
)site
folder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change
)yarn lint:check
) and fix any issues? (yarn lint:write
)PR-Codex overview
Detailed summary
PortalSigner
to the@alchemy/aa-signers
package.packages/signers/package.json
to include@portal-hq/web
as a dependency.PortalAuthenticationParams
andPortalUserInfo
interfaces topackages/signers/src/portal/types.ts
.templates/typescript/base.json
to include"dom"
in thelib
array and"es2021"
as thetarget
in thecompilerOptions
.site/.vitepress/config.ts
to include a new section forPortal Signer
in the navigation menu.getAddress
,signMessage
,getAuthDetails
, andauthenticate
methods ofPortalSigner
insite/packages/aa-signers/portal
directory.site/packages/aa-signers/portal/guides/portal.md
with installation instructions for@portal-hq/web
SDK and updated code snippets.PortalSigner
class topackages/signers/src/portal/signer.ts
with methods forgetAddress
,signMessage
,signTypedData
, andauthenticate
.