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: externally_connectable CAIP delivery and enveloping #25075

Merged
merged 57 commits into from
Jun 27, 2024

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jun 5, 2024

Description

Implements initial Delivery CAIP specs.

  • Adds BARAD_DUR feature flag
  • Adds http://* and https://* wildcards to the externally_connectable manifest property behind feature flag
  • Enables externally_connectable CAIP envelope handling from webpage connections (leaves extension connections as is) behind feature flag
  • Bifurcates the EIP1993 and CAIP RPC pipelines
    • CAIP RPC Pipeline always responds with error CAIP RPC Pipeline not yet implemented.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2528
See: jiexi/CAIPs#1

Manual testing steps

  1. BARAD_DUR=1 yarn start
  2. Use your local extension build on Chrome
  3. Go to a webpage
  4. In console enter the following, replacing EXTENSION_ID with your local extension ID
const EXTENSION_ID = 'nonfpcflonapegmnfeafnddgdniflbnk';
const extensionPort = chrome.runtime.connect(EXTENSION_ID)
extensionPort.onMessage.addListener((msg) => console.log('extensionPort on message', msg))
extensionPort.postMessage({type: 'caip-x', data: {method: 'eth_chainId'}})    
  1. There should be a log with the error response CAIP RPC Pipeline not yet implemented.

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Jun 5, 2024

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.

@jiexi jiexi changed the title Jl/mmp 2528/externally connectable caip enveloping xternally_connectable caip enveloping Jun 5, 2024
@jiexi jiexi changed the title xternally_connectable caip enveloping externally_connectable CAIP delivery and enveloping Jun 5, 2024
Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if it should be its own new package in core caip-stream? Otherwise I think json-rpc-middleware-stream could be a decent place to park it and we could consider renaming that package to be a bit more generic? @mcmire @Gudahtt

Copy link
Contributor

@mcmire mcmire Jun 7, 2024

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Comment on lines 4 to 24
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();
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiexi jiexi marked this pull request as ready for review June 6, 2024 18:56
@jiexi jiexi requested a review from kumavis as a code owner June 6, 2024 18:56
@metamaskbot
Copy link
Collaborator

Builds ready [d27526a]
Page Load Metrics (225 ± 248 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6413988168
domContentLoaded9351352
load411931225517248
domInteractive9351352
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.58 KiB (0.07%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

adonesky1
adonesky1 previously approved these changes Jun 25, 2024
Copy link
Contributor

@adonesky1 adonesky1 left a 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 🥳

@jiexi jiexi requested review from Gudahtt June 25, 2024 20:58
@metamaskbot
Copy link
Collaborator

Builds ready [1ced8f3]
Page Load Metrics (131 ± 171 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6510484105
domContentLoaded9261142
load391681131356171
domInteractive9261142
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.58 KiB (0.08%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

…nveloping

# Conflicts:
#	app/scripts/background.js
@jiexi
Copy link
Contributor Author

jiexi commented Jun 26, 2024

Including stream bifurcation in this PR as discussed with @shanejonas

@metamaskbot
Copy link
Collaborator

Builds ready [323ab1e]
Page Load Metrics (54 ± 13 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint64292955828
domContentLoaded9111010
load38149542813
domInteractive9111010
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.58 KiB (0.08%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [3c8b832]
Page Load Metrics (149 ± 207 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint631018094
domContentLoaded9201231
load402024149430207
domInteractive9201231
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 3.54 KiB (0.11%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

if (port.sender.tab?.id && process.env.BARAD_DUR) {
connectExternalCaip(...args);
} else {
connectExternalExtension(...args);
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

jiexi and others added 4 commits June 26, 2024 14:07
Co-authored-by: Shane <jonas.shane@gmail.com>
…le-caip-enveloping' into jl/mmp-2528/externally_connectable-caip-enveloping
},
);

// Used to show wallet liveliness to the provider
Copy link
Contributor

Choose a reason for hiding this comment

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

total nit

Suggested change
// Used to show wallet liveliness to the provider
// Used to signal wallet liveness to the provider

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [c8ce2f8]
Page Load Metrics (50 ± 8 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6913881157
domContentLoaded8121010
load3912150178
domInteractive8121010
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 3.37 KiB (0.10%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@jiexi jiexi merged commit 932c725 into develop Jun 27, 2024
70 checks passed
@jiexi jiexi deleted the jl/mmp-2528/externally_connectable-caip-enveloping branch June 27, 2024 15:42
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-extension-platform team-wallet-api-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants