-
Notifications
You must be signed in to change notification settings - Fork 5k
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: externally_connectable
CAIP delivery and enveloping
#25075
feat: externally_connectable
CAIP delivery and enveloping
#25075
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
externally_connectable
CAIP delivery and enveloping
app/scripts/inpage.js
Outdated
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.
the changes in this file need to be moved into the test-dapp. Only here for local testing/verification and to demonstrate how the CAIP Stream pipeline in the inpage provider is exactly the same one used in background
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.
TODO: Revert this before merge
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.
relevant poc test-dapp that enables externally_connectable without caip-x enveloping
https://github.com/MetaMask/test-dapp/pull/317/files
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.
where should this file live? My first thought is @metamask/providers
, but it's more JSON-RPC pipeline aligned. Yet it doesn't quite belong in json-rpc-middleware-stream
either, but I think that's the best place to park this. Thoughts?
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.
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.
Are we thinking of extracting this to a package because we need to share it?
If so, I tend to like small packages that do one thing, so from a responsibility perspective I'm leaning slightly toward the option of making a new package called caip-stream
instead of bundling it with json-rpc-middleware-stream
and renaming that. I could be convinced of the other option, though.
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.
How this would be used in test dapp MetaMask/test-dapp#342
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.
core PR here: MetaMask/core#4399
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.
this logic has been simplified quite a bit now and it's questionable whether this requires a new package again. If we build out a new lean provider in test-dapp to handle externally_connectable, then we don't. If we reuse our existing provider in test-dapp to handle externally_connectable, then we probably do.
shared/modules/caip-stream.ts
Outdated
export class SplitStream extends Duplex { | ||
substream: Duplex; | ||
|
||
constructor(substream?: SplitStream) { | ||
super({ objectMode: true }); | ||
this.substream = substream ?? new SplitStream(this); | ||
} | ||
|
||
_read() { | ||
return undefined; | ||
} | ||
|
||
_write( | ||
value: unknown, | ||
_encoding: BufferEncoding, | ||
callback: (error?: Error | null) => void, | ||
) { | ||
this.substream.push(value); | ||
callback(); | ||
} | ||
} |
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.
Builds ready [d27526a]
Page Load Metrics (225 ± 248 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
LGTM! Also tested and works well 🥳
Builds ready [1ced8f3]
Page Load Metrics (131 ± 171 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…nveloping # Conflicts: # app/scripts/background.js
Including stream bifurcation in this PR as discussed with @shanejonas |
Builds ready [323ab1e]
Page Load Metrics (54 ± 13 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [3c8b832]
Page Load Metrics (149 ± 207 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
if (port.sender.tab?.id && process.env.BARAD_DUR) { | ||
connectExternalCaip(...args); | ||
} else { | ||
connectExternalExtension(...args); |
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 we need the legacy API in all cases
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.
To be clear, there are 3 APIs now. Legacy, Current/EIP1193, CAIP
When you say Legacy, I assume you are referring to the Current/EIP1193.
I don't believe we are serving EIP1193 over externally_connectable connections from webpages. @adonesky1
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. @shanejonas just to clarify, this browser.runtime.onConnectExternal.addListener(...
block only for externally_connectable connections and we don't need to serve the EIP1193
provider to any consumers over that transport
Co-authored-by: Shane <[email protected]>
…le-caip-enveloping' into jl/mmp-2528/externally_connectable-caip-enveloping
}, | ||
); | ||
|
||
// Used to show wallet liveliness to the provider |
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.
total nit
// Used to show wallet liveliness to the provider | |
// Used to signal wallet liveness to the provider |
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.
LGTM!
Builds ready [c8ce2f8]
Page Load Metrics (50 ± 8 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Implements initial Delivery CAIP specs.
BARAD_DUR
feature flaghttp://*
andhttps://*
wildcards to theexternally_connectable
manifest property behind feature flagexternally_connectable
CAIP envelope handling from webpage connections (leaves extension connections as is) behind feature flagCAIP RPC Pipeline not yet implemented.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2528
See: jiexi/CAIPs#1
Manual testing steps
BARAD_DUR=1 yarn start
CAIP RPC Pipeline not yet implemented.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist