-
Notifications
You must be signed in to change notification settings - Fork 53
Prolink RPC Link Format Implementation #158
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
base: master
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
|
packages/account-sdk/src/index.ts
Outdated
| export { decodeProlink, encodeProlink } from './interface/public-utilities/prolink/index.js'; | ||
| export type { ProlinkDecoded, ProlinkRequest } from './interface/public-utilities/prolink/index.js'; | ||
|
|
||
| export { showProlinkDialog } from './ui/ProlinkDialog/index.js'; |
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.
new exports
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.
removed showProlinkDialog
|
Whats the prompt to store this in the account sdk? This feels like a separate package that account sdk could likely consume? |
| */ | ||
| export function hexToBytes(hex: string): Uint8Array { | ||
| const normalized = hex.toLowerCase().replace(/^0x/, ''); | ||
| const bytes = new Uint8Array(normalized.length / 2); |
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.
If hex is "0x123", then normalized.length = 3, and bytes = new Uint8Array(1.5) becomes new Uint8Array(1), so we will lose data here.
I would recommend using Bytes, Base64, etc from Ox for all these utilities fwiw since they are battle-tested and more performant.
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.
You're right, odd length hex is broken here. Adding test cases for coverage then applying a fix.
Dm'd you the context here |
examples/testapp/next.config.mjs
Outdated
| webpack: (config, { isServer }) => { | ||
| // Exclude Node.js built-in modules from client bundle | ||
| if (!isServer) { | ||
| config.resolve.fallback = { | ||
| ...config.resolve.fallback, | ||
| zlib: false, | ||
| }; | ||
|
|
||
| // Ignore node:zlib imports in client-side code | ||
| config.plugins.push( | ||
| new webpack.IgnorePlugin({ | ||
| resourceRegExp: /^node:zlib$/, | ||
| }) | ||
| ); | ||
| } | ||
| return config; | ||
| }, |
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.
assuming consumers of our SDK don't have to do 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.
Updated to Implement environment specific exports (and removed this)
| */ | ||
| function detectSpendPermission(typedData: TypedData): boolean { | ||
| return ( | ||
| typedData.primaryType === 'SpendPermission' && typedData.domain.verifyingContract !== undefined |
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.
should we not be checking for permission manager contract here?
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.
I don't think so. Another implementation of a permission manager contract could still be valid in this context
| // Use browser's built-in btoa for base64 encoding | ||
| let binary = ''; | ||
| for (let i = 0; i < data.length; i++) { | ||
| binary += String.fromCharCode(data[i]); | ||
| } | ||
| const base64 = btoa(binary); |
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.
nit: any reason not to use const base64String = Buffer.from(originalString).toString('base64'); here? afaik btoa is deprecated
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.
I think btoa is standard, and Buffer.from is not a native web api
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.
+1 Buffer is not available in web
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 used in the playground anywhere to see what it looks like?
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.
I removed this for now - we may add it later if requested by devs
5a363ab to
6aac8bf
Compare
6aac8bf to
026315d
Compare
|
@spencerstock, I'm not convinced this belongs in the SDK. I could see this being a separate package that the core SDK can consume. Would be curious if any others feel the same or differently |
I think the answer remains to be seen depending on what the protocol does and the types of investments we put into it in the future. But one sentiment Wilson is aligned with is that devs want more atomic control of what is displayed instead of delegating control under the hood. I think in the future where this becomes a more well-rounded solution to present requests it belongs in the base account package. It's ambiguous for now though and I'd default to just putting it in with the other utils. |
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.
agree looking from this PR this doesn't seem to be the best place for prolink
but from the sync w builder team this morning, sounds like
- this is not final, and is for unblocking devs in this interim period before Bruno's standalone lib is live
- we may migrate to import from standalone lib and convert this into a branded flow (?)
stamping to unblock, let's do quick iteration on our offering
3ba2525 to
48f4fcf
Compare
What changed? Why?
Implements the Compressed RPC Link Format (Prolink) specification for encoding wallet RPC requests into compact, URL-safe payloads suitable for QR codes, NFC tags, and deep links.
Matches the spec defined here: ethereum/ERCs#1261
Core Implementation
Shortcuts
Shortcut 0 (Generic JSON-RPC): Universal fallback for any RPC method
Shortcut 1 (wallet_sendCalls): Optimized for EIP-5792 with specialized encodings for:
Shortcut 2 (wallet_sign): Optimized for EIP-7871 with specialized encodings for:
Features
How was this tested?
Test coverage for:
How can reviewers manually test these changes?
/prolink-playgroundDemo/screenshots
Screen.Recording.2025-10-17.at.2.37.10.PM.mov