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.

Sign up for GitHub

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

remove methods from multichain API #25841

Merged
merged 4 commits into from
Jul 16, 2024
Merged

remove methods from multichain API #25841

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a76dd8
remove methods from multichain API
adonesky1 Jul 15, 2024
25386d1
add back some hooks that we still need for a bit
adonesky1 Jul 15, 2024
7289b04
adjust naming
adonesky1 Jul 15, 2024
e2c1f68
fix tests + lint
adonesky1 Jul 15, 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
26 changes: 21 additions & 5 deletions app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,33 @@ import { permissionRpcMethods } from '@metamask/permission-controller';
import { selectHooks } from '@metamask/snaps-rpc-methods';
import { hasProperty } from '@metamask/utils';
import { ethErrors } from 'eth-rpc-errors';
import { handlers as localHandlers, legacyHandlers } from './handlers';
import {
handlers as localHandlers,
legacyHandlers,
ethAccountsHandler,
} from './handlers';

const allHandlers = [...localHandlers, ...permissionRpcMethods.handlers];
const allHandlers = [
...localHandlers,
...legacyHandlers,
...permissionRpcMethods.handlers,
ethAccountsHandler,
];

// The primary home of RPC method implementations in MetaMask. MUST be subsequent
// The primary home of RPC method implementations for the injected provider API (pre-multichain). MUST be subsequent
// to our permissioning logic in the JSON-RPC middleware pipeline.
export const createMethodMiddleware = makeMethodMiddlewareMaker(allHandlers);
export const createLegacyMethodMiddleware =
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on something like this? Legacy already means something already, so doing this increases potential confusion

Suggested change
export const createLegacyMethodMiddleware =
export const createEip1193MethodMiddleware =

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 I considered this as well. Yeah we could go with this.

makeMethodMiddlewareMaker(allHandlers);

// The primary home of RPC method implementations for the MultiChain API.
export const createMultichainMethodMiddleware = makeMethodMiddlewareMaker([
...localHandlers,
ethAccountsHandler,
]);

// A collection of RPC method implementations that, for legacy reasons, MAY precede
// our permissioning logic in the JSON-RPC middleware pipeline.
export const createLegacyMethodMiddleware =
export const createEthAccountsMethodMiddleware =
makeMethodMiddlewareMaker(legacyHandlers);

/**
Expand Down
5 changes: 3 additions & 2 deletions app/scripts/lib/rpc-method-middleware/handlers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const handlers = [
logWeb3ShimUsage,
requestAccounts,
sendMetadata,
switchEthereumChain,
watchAsset,
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
mmiAuthenticate,
Expand All @@ -34,4 +33,6 @@ export const handlers = [
///: END:ONLY_INCLUDE_IF
];

export const legacyHandlers = [ethAccounts];
export const legacyHandlers = [switchEthereumChain];

export const ethAccountsHandler = ethAccounts;
32 changes: 12 additions & 20 deletions app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,9 @@ import AccountTracker from './lib/account-tracker';
import createDupeReqFilterStream from './lib/createDupeReqFilterStream';
import createLoggerMiddleware from './lib/createLoggerMiddleware';
import {
createEthAccountsMethodMiddleware,
createLegacyMethodMiddleware,
createMethodMiddleware,
createMultichainMethodMiddleware,
createUnsupportedMethodMiddleware,
} from './lib/rpc-method-middleware';
import createOriginMiddleware from './lib/createOriginMiddleware';
Expand Down Expand Up @@ -5264,10 +5265,10 @@ export default class MetamaskController extends EventEmitter {

engine.push(createUnsupportedMethodMiddleware());

// Legacy RPC methods that need to be implemented _ahead of_ the permission
// Legacy RPC method that needs to be implemented _ahead of_ the permission
// middleware.
engine.push(
createLegacyMethodMiddleware({
createEthAccountsMethodMiddleware({
getAccounts: this.getPermittedAccounts.bind(this, origin),
}),
);
Expand Down Expand Up @@ -5303,7 +5304,7 @@ export default class MetamaskController extends EventEmitter {
// Unrestricted/permissionless RPC method implementations.
// They must nevertheless be placed _behind_ the permission middleware.
engine.push(
createMethodMiddleware({
createLegacyMethodMiddleware({
origin,

subjectType,
Expand Down Expand Up @@ -5649,9 +5650,8 @@ export default class MetamaskController extends EventEmitter {
});
engine.push(requestQueueMiddleware);

// TODO: remove switchChain here
engine.push(
createMethodMiddleware({
createMultichainMethodMiddleware({
origin,

subjectType: SubjectType.Website, // TODO: this should probably be passed in
Expand Down Expand Up @@ -5690,12 +5690,14 @@ export default class MetamaskController extends EventEmitter {
this.permissionController,
origin,
),
// TODO remove this hook
requestAccountsPermission:
this.permissionController.requestPermissions.bind(
this.permissionController,
{ origin },
{ eth_accounts: {} },
),
// TODO remove this hook
requestPermittedChainsPermission: (chainIds) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dang seems like we can't remove this one yet either

this.permissionController.requestPermissions(
{ origin },
Expand All @@ -5709,25 +5711,12 @@ export default class MetamaskController extends EventEmitter {
},
},
),
// TODO remove this hook
requestPermissionsForOrigin:
this.permissionController.requestPermissions.bind(
this.permissionController,
{ origin },
),
revokePermissionsForOrigin: (permissionKeys) => {
try {
this.permissionController.revokePermissions({
[origin]: permissionKeys,
});
} catch (e) {
// we dont want to handle errors here because
// the revokePermissions api method should just
// return `null` if the permissions were not
// successfully revoked or if the permissions
// for the origin do not exist
console.log(e);
}
},
getCaveat: ({ target, caveatType }) => {
try {
return this.permissionController.getCaveat(
Expand All @@ -5746,8 +5735,10 @@ export default class MetamaskController extends EventEmitter {

return undefined;
},
// TODO refactor `add-ethereum-chain` handler so that this hook can be removed from multichain middleware
getChainPermissionsFeatureFlag: () =>
Boolean(process.env.CHAIN_PERMISSIONS),
// TODO refactor `add-ethereum-chain` handler so that this hook can be removed from multichain middleware
getCurrentRpcUrl: () =>
this.networkController.state.providerConfig.rpcUrl,
// network configuration-related
Expand All @@ -5772,6 +5763,7 @@ export default class MetamaskController extends EventEmitter {
}
},
findNetworkConfigurationBy: this.findNetworkConfigurationBy.bind(this),
// TODO refactor `add-ethereum-chain` handler so that this hook can be removed from multichain middleware
getCurrentChainIdForDomain: (domain) => {
const networkClientId =
this.selectedNetworkController.getNetworkClientIdForDomain(domain);
Expand Down
Loading