-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
fix: update transaction controllers to use selected account #4244
Changes from 4 commits
b874e4c
598275b
1f0f698
45a3819
df896e5
14db983
a4f995c
ee06e60
fb0ea80
a0f55fe
caf2f08
31141a5
452e3b3
1b6d0e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -25,6 +25,7 @@ import type { | |||||
FetchGasFeeEstimateOptions, | ||||||
GasFeeState, | ||||||
} from '@metamask/gas-fee-controller'; | ||||||
import type { InternalAccount } from '@metamask/keyring-api'; | ||||||
import type { | ||||||
BlockTracker, | ||||||
NetworkClientId, | ||||||
|
@@ -297,7 +298,7 @@ export type TransactionControllerOptions = { | |||||
getNetworkState: () => NetworkState; | ||||||
getPermittedAccounts: (origin?: string) => Promise<string[]>; | ||||||
getSavedGasFees?: (chainId: Hex) => SavedGasFees | undefined; | ||||||
getSelectedAddress: () => string; | ||||||
getSelectedAccount: () => InternalAccount; | ||||||
incomingTransactions?: IncomingTransactionOptions; | ||||||
isMultichainEnabled: boolean; | ||||||
isSimulationEnabled?: () => boolean; | ||||||
|
@@ -614,7 +615,7 @@ export class TransactionController extends BaseController< | |||||
|
||||||
private readonly getPermittedAccounts: (origin?: string) => Promise<string[]>; | ||||||
|
||||||
private readonly getSelectedAddress: () => string; | ||||||
private readonly getSelectedAccount: () => InternalAccount; | ||||||
|
||||||
private readonly getExternalPendingTransactions: ( | ||||||
address: string, | ||||||
|
@@ -733,7 +734,7 @@ export class TransactionController extends BaseController< | |||||
* @param options.getNetworkState - Gets the state of the network controller. | ||||||
* @param options.getPermittedAccounts - Get accounts that a given origin has permissions for. | ||||||
* @param options.getSavedGasFees - Gets the saved gas fee config. | ||||||
* @param options.getSelectedAddress - Gets the address of the currently selected account. | ||||||
* @param options.getSelectedAccount - Gets the currently selected account. | ||||||
* @param options.incomingTransactions - Configuration options for incoming transaction support. | ||||||
* @param options.isMultichainEnabled - Enable multichain support. | ||||||
* @param options.isSimulationEnabled - Whether new transactions will be automatically simulated. | ||||||
|
@@ -761,7 +762,7 @@ export class TransactionController extends BaseController< | |||||
getNetworkState, | ||||||
getPermittedAccounts, | ||||||
getSavedGasFees, | ||||||
getSelectedAddress, | ||||||
getSelectedAccount, | ||||||
incomingTransactions = {}, | ||||||
isMultichainEnabled = false, | ||||||
isSimulationEnabled, | ||||||
|
@@ -802,7 +803,7 @@ export class TransactionController extends BaseController< | |||||
this.getGasFeeEstimates = | ||||||
getGasFeeEstimates || (() => Promise.resolve({} as GasFeeState)); | ||||||
this.getPermittedAccounts = getPermittedAccounts; | ||||||
this.getSelectedAddress = getSelectedAddress; | ||||||
this.getSelectedAccount = getSelectedAccount; | ||||||
this.getExternalPendingTransactions = | ||||||
getExternalPendingTransactions ?? (() => []); | ||||||
this.securityProviderRequest = securityProviderRequest; | ||||||
|
@@ -1035,7 +1036,7 @@ export class TransactionController extends BaseController< | |||||
if (origin) { | ||||||
await validateTransactionOrigin( | ||||||
await this.getPermittedAccounts(origin), | ||||||
this.getSelectedAddress(), | ||||||
this.getSelectedAccount().address, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this controller takes a function that allows for getting the currently selected account. However, this is an old pattern and we are slowly trying to move all controllers to use the messenger pattern for intercommunication. Since you're making a breaking change anyway, what are your thoughts on extending the TransactionControllerMessenger to allow the
Suggested change
|
||||||
txParams.from, | ||||||
origin, | ||||||
); | ||||||
|
@@ -3430,7 +3431,7 @@ export class TransactionController extends BaseController< | |||||
}): IncomingTransactionHelper { | ||||||
const incomingTransactionHelper = new IncomingTransactionHelper({ | ||||||
blockTracker, | ||||||
getCurrentAccount: this.getSelectedAddress, | ||||||
getCurrentAccount: this.getSelectedAccount, | ||||||
getLastFetchedBlockNumbers: () => this.state.lastFetchedBlockNumbers, | ||||||
getChainId: chainId ? () => chainId : this.getChainId.bind(this), | ||||||
isEnabled: this.#incomingTransactionOptions.isEnabled, | ||||||
|
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.
Noting that this is a type-only import and not under
peerDependencies
, this is a something that could be moved todevDependencies
?(If strict compatibility with the imported class is required, that indicates to me that it could also be a candidate for
peerDependencies
, if introducing that requirement here is indeed intended)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.
Looking at the emitted type declaration file, it looks like the
@metamask/keyring-api
shows up there, so I believe this needs to be independencies
, notdevDependencies
orpeerDependencies
so that it doesn't create a TypeScript error on build. Right?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.
Unrelated, it looks like this is missing a
^
: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.
@mcmire I may be missing something but it seems to me that if we just consider the
peerDependencies
aspect in isolation, it should also be here.And if the
peerDependencies
is satisifed, then there should be no TypeScript error on build, and therefore no need fordependencies
since the types will be provided by thepeerDependencies
entry?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'm thinking if it's not added in
peerDependencies
, using code may run the risk of getting inconsistent type-checking results or plain incompatibilities, due to using the wrong version of@metamask/keyring-api
for its type-checking?Specifically here, it would be wrt the
getSelectedAccount
constructor option.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've removed the callback and used the messaging system instead to get the selected account.
@metamask/keyring-api
can just be a dev dependency for the tests.