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
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
e0c7edb
WIP
jiexi Jun 3, 2024
370dc06
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 3, 2024
c1ae3db
WIP
jiexi Jun 5, 2024
61e9697
WIP PortStream bypass sanity check (working)
jiexi Jun 5, 2024
1f8cfd2
WIP wrapped stream (working)
jiexi Jun 5, 2024
5db9a97
cleanup inpage
jiexi Jun 5, 2024
6ef86a4
DRY caip stream
jiexi Jun 5, 2024
97469f9
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 5, 2024
1f58e7e
Rename. WIP spec
jiexi Jun 5, 2024
a9b741f
add SplitStream specs
jiexi Jun 5, 2024
7df64f3
add CaipToMultiplexStream, MultiplexToCaipStream specs
jiexi Jun 5, 2024
7db9d75
lint
jiexi Jun 5, 2024
65c9d68
WIP createCaipStream spec
jiexi Jun 5, 2024
c8b66b5
add createCaipStream specs
jiexi Jun 6, 2024
bbb33ae
lint
jiexi Jun 6, 2024
edab0fe
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 6, 2024
040d300
add BARAD_DUR flag
jiexi Jun 6, 2024
2acb719
dry background trackDappView
jiexi Jun 6, 2024
9792594
move externally_connectable manifest wildcard behind BARAD_DUR
jiexi Jun 6, 2024
7372604
jsdoc
jiexi Jun 6, 2024
b82888b
restore inpage
jiexi Jun 11, 2024
64ca655
Move caip stream closer to provider. Replace caip<->multiplex transfo…
jiexi Jun 11, 2024
714d044
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 11, 2024
29ea68c
Rename connectExternalDapp to connectExternalCaip
jiexi Jun 11, 2024
64ba990
actually restore inpage
jiexi Jun 11, 2024
46bfee7
Fix createCaipStream specs
jiexi Jun 11, 2024
e24ef63
use createDeferredPromise instead
jiexi Jun 11, 2024
379ce8a
rename onData to readFromStream
jiexi Jun 12, 2024
98ff148
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 12, 2024
cd3dd02
fix method names. add setupUntrustedCommunicationCaip
jiexi Jun 12, 2024
7f1eb8a
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 12, 2024
cbfd014
lint
jiexi Jun 12, 2024
5d65634
Merge remote-tracking branch 'origin/jl/mmp-2528/externally_connectab…
jiexi Jun 12, 2024
3652ab3
lint
jiexi Jun 12, 2024
a239e64
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 13, 2024
4f4f999
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 14, 2024
5b667f2
Rename connectExternalLegacy to connectExternalExtension
jiexi Jun 14, 2024
0c3f000
rename _subjectType to inputSubjectType
jiexi Jun 14, 2024
ab7767b
use messenger where possible
jiexi Jun 18, 2024
7fc0b23
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 18, 2024
ccd10ab
Rename setupUntrustedCommunicationLegacy to setupUntrustedCommunicati…
jiexi Jun 18, 2024
2eeac73
lint
jiexi Jun 18, 2024
600daed
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 21, 2024
ee4dec5
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 21, 2024
dc6c189
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 21, 2024
d27526a
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 24, 2024
571cf9e
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 25, 2024
1ced8f3
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 25, 2024
487b9ca
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 26, 2024
323ab1e
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 26, 2024
7d3d5f6
Bifurcate streams
jiexi Jun 26, 2024
2258ee7
Merge branch 'develop' into jl/mmp-2528/externally_connectable-caip-e…
jiexi Jun 26, 2024
3c8b832
fix specs
jiexi Jun 26, 2024
439f27e
Update app/scripts/metamask-controller.js
jiexi Jun 26, 2024
9f35203
remove requestAccountTabIds for CAIP
jiexi Jun 26, 2024
fa34c32
Merge remote-tracking branch 'origin/jl/mmp-2528/externally_connectab…
jiexi Jun 26, 2024
c8ce2f8
lint
jiexi Jun 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/manifest/v2/_barad_dur.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"externally_connectable": {
"matches": ["http://*/*", "https://*/*"],
"ids": ["*"]
}
}
6 changes: 6 additions & 0 deletions app/manifest/v3/_barad_dur.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"externally_connectable": {
"matches": ["http://*/*", "https://*/*"],
"ids": ["*"]
}
}
124 changes: 97 additions & 27 deletions app/scripts/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ const sendReadyMessageToTabs = async () => {

// These are set after initialization
let connectRemote;
let connectExternal;
let connectExternalLegacy;
let connectExternalCaip;

browser.runtime.onConnect.addListener(async (...args) => {
// Queue up connection attempts here, waiting until after initialization
Expand All @@ -191,7 +192,13 @@ browser.runtime.onConnectExternal.addListener(async (...args) => {
// Queue up connection attempts here, waiting until after initialization
await isInitialized;
// This is set in `setupController`, which is called as part of initialization
connectExternal(...args);
const port = args[0];

if (port.sender.tab?.id && process.env.BARAD_DUR) {
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
connectExternalCaip(...args);
} else {
connectExternalLegacy(...args);
}
});

function saveTimestamp() {
Expand Down Expand Up @@ -496,6 +503,47 @@ function emitDappViewedMetricEvent(
}
}

/**
* Track dapp connection when loaded and permissioned
*
* @param {Port} remotePort - The port provided by a new context.
* @param {object} preferencesController - Preference Controller to get total created accounts
* @param {object} permissionController - Permission Controller to check if origin is permitted
*/
function trackDappView(
remotePort,
preferencesController,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We've been trying to move away from passing around controller references as arguments, as we found it makes it easy to miss things when making breaking changes to controllers.

I see that this is pre-existing in this case though, so maybe we can clean this up later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the preference to pass a callback in instead?

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. The preferences is to use the messenger, or callbacks for code not setup with a messenger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the messenger where possible now ab7767b

permissionController,
) {
if (!remotePort.sender || !remotePort.sender.tab || !remotePort.sender.url) {
return;
}
const tabId = remotePort.sender.tab.id;
const url = new URL(remotePort.sender.url);
const { origin } = url;

// store the orgin to corresponding tab so it can provide infor for onActivated listener
if (!Object.keys(tabOriginMapping).includes(tabId)) {
tabOriginMapping[tabId] = origin;
}
const connectSitePermissions = permissionController.state.subjects[origin];
// when the dapp is not connected, connectSitePermissions is undefined
const isConnectedToDapp = connectSitePermissions !== undefined;
// when open a new tab, this event will trigger twice, only 2nd time is with dapp loaded
const isTabLoaded = remotePort.sender.tab.title !== 'New Tab';

// *** Emit DappViewed metric event when ***
// - refresh the dapp
// - open dapp in a new tab
if (isConnectedToDapp && isTabLoaded) {
emitDappViewedMetricEvent(
origin,
connectSitePermissions,
preferencesController,
);
}
}

/**
* Initializes the MetaMask Controller with any initial state and default language.
* Configures platform-specific error reporting strategy.
Expand Down Expand Up @@ -693,27 +741,11 @@ export function setupController(
const url = new URL(remotePort.sender.url);
const { origin } = url;

// store the orgin to corresponding tab so it can provide infor for onActivated listener
if (!Object.keys(tabOriginMapping).includes(tabId)) {
tabOriginMapping[tabId] = origin;
}
const connectSitePermissions =
controller.permissionController.state.subjects[origin];
// when the dapp is not connected, connectSitePermissions is undefined
const isConnectedToDapp = connectSitePermissions !== undefined;
// when open a new tab, this event will trigger twice, only 2nd time is with dapp loaded
const isTabLoaded = remotePort.sender.tab.title !== 'New Tab';

// *** Emit DappViewed metric event when ***
// - refresh the dapp
// - open dapp in a new tab
if (isConnectedToDapp && isTabLoaded) {
emitDappViewedMetricEvent(
origin,
connectSitePermissions,
controller.preferencesController,
);
}
trackDappView(
remotePort,
controller.preferencesController,
controller.permissionController,
);

remotePort.onMessage.addListener((msg) => {
if (
Expand All @@ -724,22 +756,60 @@ export function setupController(
}
});
}
connectExternal(remotePort);
connectExternalLegacy(remotePort);
}
};

// communication with page or other extension
connectExternal = (remotePort) => {
connectExternalLegacy = (remotePort) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe we could call this connectExternalExtension instead, to better reflect what it does. Even after we ship this, it won't really be a legacy feature.

Copy link
Member

Choose a reason for hiding this comment

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

I guess technically it was also used for the desktop connection, though that has since been removed

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 good point. Also, we have an existing legacy stream already haha. 5b667f2

const portStream =
overrides?.getPortStream?.(remotePort) || new PortStream(remotePort);
controller.setupUntrustedCommunicationLegacy({
connectionStream: portStream,
sender: remotePort.sender,
});
};

connectExternalCaip = async (remotePort) => {
if (metamaskBlockedPorts.includes(remotePort.name)) {
return;
}

// this is triggered when a new tab is opened, or origin(url) is changed
if (remotePort.sender && remotePort.sender.tab && remotePort.sender.url) {
const tabId = remotePort.sender.tab.id;
const url = new URL(remotePort.sender.url);
const { origin } = url;

trackDappView(
remotePort,
controller.preferencesController,
controller.permissionController,
);

// TODO: remove this when we separate the legacy and multichain rpc pipelines
Copy link
Member

Choose a reason for hiding this comment

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

Curious to know what this refers to, i.e. what does it mean to "separate the legacy and multichain RPC pipelines", and how is this requestAccountTabIds state related?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Gudahtt we are intending to create a separate JSON RPC pipeline for the new API (ticket here) for long term maintainability

Copy link
Contributor

Choose a reason for hiding this comment

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

@Gudahtt just want to confirm you've seen and understood this roadmap?

remotePort.onMessage.addListener((msg) => {
if (
msg.type === 'caip-x' &&
jiexi marked this conversation as resolved.
Show resolved Hide resolved
msg.data &&
msg.data.method === MESSAGE_TYPE.ETH_REQUEST_ACCOUNTS
) {
requestAccountTabIds[origin] = tabId;
}
});
}

const portStream =
overrides?.getPortStream?.(remotePort) || new PortStream(remotePort);
controller.setupUntrustedCommunication({

controller.setupUntrustedCommunicationCaip({
connectionStream: portStream,
sender: remotePort.sender,
});
};

if (overrides?.registerConnectListeners) {
overrides.registerConnectListeners(connectRemote, connectExternal);
overrides.registerConnectListeners(connectRemote, connectExternalLegacy);
}

//
Expand Down
30 changes: 28 additions & 2 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ import {
getSmartTransactionsOptInStatus,
getCurrentChainSupportsSmartTransactions,
} from '../../shared/modules/selectors';
import { createCaipStream } from '../../shared/modules/caip-stream';
import {
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
handleMMITransactionUpdate,
Expand Down Expand Up @@ -4783,7 +4784,7 @@ export default class MetamaskController extends EventEmitter {
* @param {MessageSender | SnapSender} options.sender - The sender of the messages on this stream.
* @param {string} [options.subjectType] - The type of the sender, i.e. subject.
*/
setupUntrustedCommunication({ connectionStream, sender, subjectType }) {
setupUntrustedCommunicationLegacy({ connectionStream, sender, subjectType }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gudahtt do you have a suggestion for this function name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to setupUntrustedCommunicationEip1193 here ccd10ab

const { completedOnboarding } = this.onboardingController.store.getState();
const { usePhishDetect } = this.preferencesController.store.getState();

Expand Down Expand Up @@ -4831,6 +4832,31 @@ export default class MetamaskController extends EventEmitter {
}
}

/**
* Used to create a CAIP stream for connecting to an untrusted context.
*
* @param options - Options bag.
* @param {ReadableStream} options.connectionStream - The Duplex stream to connect to.
* @param {MessageSender | SnapSender} options.sender - The sender of the messages on this stream.
* @param {string} [options.subjectType] - The type of the sender, i.e. subject.
*/

setupUntrustedCommunicationCaip({ connectionStream, sender, subjectType }) {
let _subjectType;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I know a leading underscore is commonly used like this to avoid shadowing, but it's a bit confusing because of the other convention we have where a leading underscore indicates an unused parameter. It sorta defeats the purpose of avoiding shadowing in general (that rule is meant to prevent confusing two variables, but here we might still mistake them for each other because of how similar the names are)

Perhaps we can alias the parameter instead? That would allow us to give it a more meaningful name to ensure it isn't used anywhere instead of _subjectType. e.g. we could call it inputSubjectType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. updated usages here 0c3f000

if (subjectType) {
_subjectType = subjectType;
} else if (sender.id && sender.id !== this.extension.runtime.id) {
_subjectType = SubjectType.Extension;
} else {
_subjectType = SubjectType.Website;
}

const caipStream = createCaipStream(connectionStream);

// messages between subject and background
this.setupProviderConnection(caipStream, sender, _subjectType);
}

/**
* Used to create a multiplexed stream for connecting to a trusted context,
* like our own user interfaces, which have the provider APIs, but also
Expand Down Expand Up @@ -5032,7 +5058,7 @@ export default class MetamaskController extends EventEmitter {
* @param connectionStream
*/
setupSnapProvider(snapId, connectionStream) {
this.setupUntrustedCommunication({
this.setupUntrustedCommunicationLegacy({
connectionStream,
sender: { snapId },
subjectType: SubjectType.Snap,
Expand Down
2 changes: 2 additions & 0 deletions builds.yml
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ env:
- BLOCKAID_PUBLIC_KEY
# Determines if feature flagged Multichain Transactions should be used
- TRANSACTION_MULTICHAIN: ''
# Determines if Barad Dur features should be used
- BARAD_DUR: ''
# Determines if feature flagged Chain permissions
- CHAIN_PERMISSIONS: ''
# Enables use of test gas fee flow to debug gas fee estimation
Expand Down
4 changes: 4 additions & 0 deletions development/build/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ const IS_MV3_ENABLED =
const baseManifest = IS_MV3_ENABLED
? require('../../app/manifest/v3/_base.json')
: require('../../app/manifest/v2/_base.json');
const baradDurManifest = IS_MV3_ENABLED
? require('../../app/manifest/v3/_barad_dur.json')
: require('../../app/manifest/v2/_barad_dur.json');
const { loadBuildTypesConfig } = require('../lib/build-type');

const { TASKS, ENVIRONMENT } = require('./constants');
Expand Down Expand Up @@ -41,6 +44,7 @@ function createManifestTasks({
);
const result = mergeWith(
cloneDeep(baseManifest),
process.env.BARAD_DUR ? cloneDeep(baradDurManifest) : {},
platformModifications,
adonesky1 marked this conversation as resolved.
Show resolved Hide resolved
browserVersionMap[platform],
await getBuildModifications(buildType, platform),
Expand Down
81 changes: 81 additions & 0 deletions shared/modules/caip-stream.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { Duplex, PassThrough } from 'readable-stream';
import { createDeferredPromise } from '@metamask/utils';
import { createCaipStream } from './caip-stream';

const writeToStream = async (stream: Duplex, message: unknown) => {
const { promise: isWritten, resolve: writeCallback } =
createDeferredPromise();

stream.write(message, () => writeCallback());
await isWritten;
};

export const readFromStream = (stream: Duplex): unknown[] => {
const chunks: unknown[] = [];
stream.on('data', (chunk: unknown) => {
chunks.push(chunk);
});

return chunks;
};

class MockStream extends Duplex {
chunks: unknown[] = [];

constructor() {
super({ objectMode: true });
}

_read() {
return undefined;
}

_write(
value: unknown,
_encoding: BufferEncoding,
callback: (error?: Error | null) => void,
) {
this.chunks.push(value);
callback();
}
}

describe('CAIP Stream', () => {
describe('createCaipStream', () => {
it('pipes and unwraps a caip-x message from source stream to the substream', async () => {
const sourceStream = new PassThrough({ objectMode: true });
const sourceStreamChunks = readFromStream(sourceStream);

const providerStream = createCaipStream(sourceStream);
const providerStreamChunks = readFromStream(providerStream);

await writeToStream(sourceStream, {
type: 'caip-x',
data: { foo: 'bar' },
});

expect(sourceStreamChunks).toStrictEqual([
{ type: 'caip-x', data: { foo: 'bar' } },
]);
expect(providerStreamChunks).toStrictEqual([{ foo: 'bar' }]);
});

it('pipes and wraps a message from the substream to the source stream', async () => {
// using a fake stream here instead of PassThrough to prevent a loop
// when sourceStream gets written back to at the end of the CAIP pipeline
const sourceStream = new MockStream();

const providerStream = createCaipStream(sourceStream);

await writeToStream(providerStream, {
foo: 'bar',
});

// Note that it's not possible to verify the output side of the internal SplitStream
// instantiated inside createCaipStream as only the substream is actually exported
expect(sourceStream.chunks).toStrictEqual([
{ type: 'caip-x', data: { foo: 'bar' } },
]);
});
});
});
Loading