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

TransactionController networkClientId updates #3652

Merged
merged 20 commits into from
Dec 13, 2023

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Dec 12, 2023

  • Update getCurrentNetworkEIP1559Compatibility hook type to accept optional networkClientId param
    • It already does this in networkController, but perhaps it should be renamed getEIP1559Compatibility?
  • Update getExternalPendingTransactions hook type to accept optional chainId param
    • SmartTransactionController and extension need to be updated
  • Add getNetworkClientIdForDomain hook to constructor param
  • Add getEthQuery method that accepts optional networkClientId and returns the correct EthQuery instance
  • Update addTransaction
    • Add optional networkClientId to options object param
    • Use correct networkClientId/chainId
  • Update stopTransaction to use correct networkClientId/chainId
  • Update estimateGas
    • Add optional networkClientId param
    • Use correct networkClientId/chainId
  • Update estimateGasBuffered to accept optional networkClientId
    • SmartTransactionController and extension need to be updated
  • Update updateGasProperties to use correct networkClientId (from txMeta)
  • Update approveTransaction to use correct networkClientId (from txMeta)
  • Update publishTransaction to accept required ethQuery
  • Update getEIP1559Compatibility to accept optional networkClientId
    • GasFeeController has a similar method of it's own that we may need to update
  • Update getNonceTrackerPendingTransactions to pass chainId to getExternalPendingTransactions
  • Add optional networkClientId to TransactionMeta type
  • Update UpdateGasRequest to accept either providerConfig or NetworkClientConfiguration
    • Probably should rename the arg from providerConfig

Questions:

  • should wipeTransactions be updated to filter by optional chainId?
  • should getNonceLock take in chainId (non-optional) to get correct nonceTracker nonceLock?
  • should getTransactions accept chainId and/or networkClientId param for filtering?
  • createApprovalsForUnapprovedTransactions isn't used anywhere in our repos? Remove it?
  • approveTransaction sets txMeta.txParams.chainId to the currentChainId. Shouldn't it read the value from txMeta instead?
  • shouldn't addExternalTransaction filter against the txMeta chainId, not the currentChainId?
  • shouldn't markNonceDuplicatesDropped filter against the txMeta chainId, not the currentChainId?

@jiexi jiexi changed the title jiexi side quest TransactionController addTransaction by networkClientId Dec 12, 2023
@jiexi jiexi self-assigned this Dec 12, 2023
@jiexi jiexi changed the title TransactionController addTransaction by networkClientId TransactionController networkClientId updates Dec 12, 2023
@@ -1912,7 +1958,7 @@ export class TransactionController extends BaseControllerV1<
/**
* Create approvals for all unapproved transactions on current chain.
*/
private createApprovalsForUnapprovedTransactions() {
private createApprovalsForUnapprovedTransactions() { // NOTE(JL): this doesn't seem to be used anywhere. Can we remove it?
Copy link
Contributor

Choose a reason for hiding this comment

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

ping Matthew/Mark

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -2109,7 +2155,7 @@ export class TransactionController extends BaseControllerV1<

transactionMeta.status = TransactionStatus.approved;
transactionMeta.txParams.nonce = nonce;
transactionMeta.txParams.chainId = chainId;
transactionMeta.txParams.chainId = chainId; // NOTE(JL): should this be coming from txMeta instead of the currentChainId?
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably can come from txMeta. Update this in another branch against main

@jiexi jiexi marked this pull request as ready for review December 13, 2023 19:50
@jiexi jiexi requested a review from a team as a code owner December 13, 2023 19:50
@jiexi jiexi merged commit a968ee7 into transaction-multichain Dec 13, 2023
@jiexi jiexi deleted the transaction-multichain-jiexi-side-quest branch December 13, 2023 20:06
shanejonas pushed a commit that referenced this pull request Dec 15, 2023
* Update `getCurrentNetworkEIP1559Compatibility` hook type to accept
optional networkClientId param
* It already does this in networkController, but perhaps it should be
renamed getEIP1559Compatibility?
* Update `getExternalPendingTransactions` hook type to accept optional
chainId param
  * SmartTransactionController and extension need to be updated
* Add `getNetworkClientIdForDomain` hook to constructor param
* Add `getEthQuery` method that accepts optional networkClientId and
returns the correct EthQuery instance
* Update `addTransaction`
  * Add optional `networkClientId` to options object param
  * Use correct networkClientId/chainId
* Update `stopTransaction` to use correct networkClientId/chainId
* Update `estimateGas`
  * Add optional `networkClientId` param
  * Use correct networkClientId/chainId
* Update `estimateGasBuffered` to accept optional networkClientId
  * SmartTransactionController and extension need to be updated 
* Update `updateGasProperties` to use correct networkClientId (from
txMeta)
* Update `approveTransaction` to use correct networkClientId (from
txMeta)
* Update `publishTransaction` to accept required ethQuery
* Update `getEIP1559Compatibility` to accept optional networkClientId
* GasFeeController has a similar method of it's own that we may need to
update
* Update `getNonceTrackerPendingTransactions` to pass `chainId` to
`getExternalPendingTransactions`
* Add optional `networkClientId` to `TransactionMeta` type
* Update `UpdateGasRequest` to accept either providerConfig or
NetworkClientConfiguration
  * Probably should rename the arg from `providerConfig`

Questions:
* should `wipeTransactions` be updated to filter by optional chainId?
* should `getNonceLock` take in chainId (non-optional) to get correct
nonceTracker nonceLock?
* should `getTransactions` accept chainId and/or networkClientId param
for filtering?
* `createApprovalsForUnapprovedTransactions` isn't used anywhere in our
repos? Remove it?
* `approveTransaction` sets txMeta.txParams.chainId to the
currentChainId. Shouldn't it read the value from txMeta instead?
* shouldn't `addExternalTransaction` filter against the txMeta chainId,
not the currentChainId?
* shouldn't `markNonceDuplicatesDropped ` filter against the txMeta
chainId, not the currentChainId?

---------

Co-authored-by: Jiexi Luan <[email protected]>
shanejonas pushed a commit that referenced this pull request Dec 15, 2023
* Update `getCurrentNetworkEIP1559Compatibility` hook type to accept
optional networkClientId param
* It already does this in networkController, but perhaps it should be
renamed getEIP1559Compatibility?
* Update `getExternalPendingTransactions` hook type to accept optional
chainId param
  * SmartTransactionController and extension need to be updated
* Add `getNetworkClientIdForDomain` hook to constructor param
* Add `getEthQuery` method that accepts optional networkClientId and
returns the correct EthQuery instance
* Update `addTransaction`
  * Add optional `networkClientId` to options object param
  * Use correct networkClientId/chainId
* Update `stopTransaction` to use correct networkClientId/chainId
* Update `estimateGas`
  * Add optional `networkClientId` param
  * Use correct networkClientId/chainId
* Update `estimateGasBuffered` to accept optional networkClientId
  * SmartTransactionController and extension need to be updated 
* Update `updateGasProperties` to use correct networkClientId (from
txMeta)
* Update `approveTransaction` to use correct networkClientId (from
txMeta)
* Update `publishTransaction` to accept required ethQuery
* Update `getEIP1559Compatibility` to accept optional networkClientId
* GasFeeController has a similar method of it's own that we may need to
update
* Update `getNonceTrackerPendingTransactions` to pass `chainId` to
`getExternalPendingTransactions`
* Add optional `networkClientId` to `TransactionMeta` type
* Update `UpdateGasRequest` to accept either providerConfig or
NetworkClientConfiguration
  * Probably should rename the arg from `providerConfig`

Questions:
* should `wipeTransactions` be updated to filter by optional chainId?
* should `getNonceLock` take in chainId (non-optional) to get correct
nonceTracker nonceLock?
* should `getTransactions` accept chainId and/or networkClientId param
for filtering?
* `createApprovalsForUnapprovedTransactions` isn't used anywhere in our
repos? Remove it?
* `approveTransaction` sets txMeta.txParams.chainId to the
currentChainId. Shouldn't it read the value from txMeta instead?
* shouldn't `addExternalTransaction` filter against the txMeta chainId,
not the currentChainId?
* shouldn't `markNonceDuplicatesDropped ` filter against the txMeta
chainId, not the currentChainId?

---------

Co-authored-by: Jiexi Luan <[email protected]>
shanejonas pushed a commit that referenced this pull request Jan 8, 2024
* Update `getCurrentNetworkEIP1559Compatibility` hook type to accept
optional networkClientId param
* It already does this in networkController, but perhaps it should be
renamed getEIP1559Compatibility?
* Update `getExternalPendingTransactions` hook type to accept optional
chainId param
  * SmartTransactionController and extension need to be updated
* Add `getNetworkClientIdForDomain` hook to constructor param
* Add `getEthQuery` method that accepts optional networkClientId and
returns the correct EthQuery instance
* Update `addTransaction`
  * Add optional `networkClientId` to options object param
  * Use correct networkClientId/chainId
* Update `stopTransaction` to use correct networkClientId/chainId
* Update `estimateGas`
  * Add optional `networkClientId` param
  * Use correct networkClientId/chainId
* Update `estimateGasBuffered` to accept optional networkClientId
  * SmartTransactionController and extension need to be updated 
* Update `updateGasProperties` to use correct networkClientId (from
txMeta)
* Update `approveTransaction` to use correct networkClientId (from
txMeta)
* Update `publishTransaction` to accept required ethQuery
* Update `getEIP1559Compatibility` to accept optional networkClientId
* GasFeeController has a similar method of it's own that we may need to
update
* Update `getNonceTrackerPendingTransactions` to pass `chainId` to
`getExternalPendingTransactions`
* Add optional `networkClientId` to `TransactionMeta` type
* Update `UpdateGasRequest` to accept either providerConfig or
NetworkClientConfiguration
  * Probably should rename the arg from `providerConfig`

Questions:
* should `wipeTransactions` be updated to filter by optional chainId?
* should `getNonceLock` take in chainId (non-optional) to get correct
nonceTracker nonceLock?
* should `getTransactions` accept chainId and/or networkClientId param
for filtering?
* `createApprovalsForUnapprovedTransactions` isn't used anywhere in our
repos? Remove it?
* `approveTransaction` sets txMeta.txParams.chainId to the
currentChainId. Shouldn't it read the value from txMeta instead?
* shouldn't `addExternalTransaction` filter against the txMeta chainId,
not the currentChainId?
* shouldn't `markNonceDuplicatesDropped ` filter against the txMeta
chainId, not the currentChainId?

---------

Co-authored-by: Jiexi Luan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants