chore: remove eth-method-registry package
#5203
Conversation
cryptodev-2s
force-pushed
the
cryptodev2s/remove-eth-method-registry-package
branch
from
1cfbf7c to
77c9cd8
Compare
cryptodev-2s
changed the title
eth-method-registry package
|
@metamaskbot publish-preview |
packages/transaction-controller/src/helpers/MethodDataHelper.ts
Outdated
Show resolved
Hide resolved
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
I am testing the changes in this PR: MetaMask/metamask-extension#29749. You can check this commit for details: 1d1f45b. I will also try to update the test to avoid mocking |
|
@cryptodev-2s One more question on this PR. I get that we want to remove dependencies on |
To answer your question, there hasn’t been any discussion about the package, nor am I aware of any plans to replace it. I made the change primarily because it removed several eth* packages at once and reduced the build size by nearly 1MB. In this case, it might make more sense to update eth-method-registry ? |
|
@cryptodev-2s Yeah, I'm thinking that it might be better to update But I guess I can see the other way too; by moving the code here we're effectively moving the package into the monorepo, and that's kind of nice from a maintainability perspective. Curious to know what the Confirmations team thinks... |
@OGPoyraz, any thoughts on the discussion regarding this change or whether we should keep eth-method-registry? move it to core ? |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@cryptodev-2s I will defer this question to @matthewwalsh0 as we discussed yesterday |
There was a problem hiding this comment.
We can all agree the TransactionController is large already, and the placement of this method data lookup within this controller has already been debated in the past.
The handleMethodData isn't used in extension, and only slightly in mobile, but that too could be removed to align with the extension code that uses token ABIs, 4Byte, Uniswap Command Parsing, and Sourcify.
I think in future the confirmations team would like to bundle all of that transaction decoding logic into a clean @metamask/transaction-decode that lives in core and is used by this controller to augment transaction metadata.
Given that intention, I'd vote not to push in the opposite direction now by adding more lower-level logic into this controller.
Instead we could continue to encapsulate via the method registry package at least for now, and then in future we could use that package within a new dedicated transaction decode package, or flatten its contents into the new package given its very small.
Explanation
This PR removes the eth-method-registry package, as it relies on @metamask/ethjs-query and @metamask/ethjs-contract, which are not EIP-1193 compatible.
References
Fixes: partially completes #5121
Changelog
Checklist