feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, watchAsset #4

krzysu · 2024-10-22T13:28:18Z

No description provided.

src/WalletRpcPlugin.ts

src/types.ts

jdevcs

also changes required in readme and package.json like:
https://github.com/web3/web3-wallet-rpc-utils/blob/a342b7bc3fd16a85509369c2b30f5a984c1e663d/package.json#L2

Thanks

krzysu · 2024-10-23T11:44:10Z

also changes required in readme and package.json like:

https://github.com/web3/web3-wallet-rpc-utils/blob/a342b7bc3fd16a85509369c2b30f5a984c1e663d/package.json#L2

Thanks

that is already covered in my very first PR https://github.com/web3/web3-wallet-rpc-utils/pull/2/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519

There are 3 PRs so far, each building on top of the other, as I try to move fast but also to keep the review scope small.

krzysu · 2024-10-23T13:31:28Z

src/utils.ts

+export function parseToGetOwnedAssetsResult(
+  result: unknown[]
+): GetOwnedAssetsResult {
+  return result as GetOwnedAssetsResult;


this is a reminder for now to parse data returned by wallet to the expected format, even if not returned by wallet in the expected format.

krzysu · 2024-10-23T15:39:10Z

src/WalletRpcPlugin.ts

+   * @param param - See {@link AddEthereumChainRequest}
+   * @returns a Promise that resolves if the request is successful
+   */
+  public async addEthereumChain(param: AddEthereumChainRequest): Promise<void> {


wdyt about removing Ethereum from the function names?
The usage would look like:

web3.walletRpc.addChain(); web3.walletRpc.switchChain(); web3.walletRpc.updateChain();

etc.

I can see arguments for both approaches, but I think it's best to maintain consistency with the RPC method names.

danforbes

Several comments/questions, but nothing blocking

danforbes · 2024-10-23T18:16:35Z

README.md

+- [wallet_watchAsset (EIP-747)](https://eips.ethereum.org/EIPS/eip-747)
+- [wallet_requestPermissions (EIP-2255)](https://eips.ethereum.org/EIPS/eip-2255)
+- [wallet_getPermissions (EIP-2255)](https://eips.ethereum.org/EIPS/eip-2255)


Same comment as before. It would be nice to mention these aren't yet implemented.

danforbes · 2024-10-23T18:18:40Z

README.md

+});
+```
+
+#### watchAsset


Can you update the PR title to mention that this method is implemented?

danforbes · 2024-10-23T18:20:21Z

src/WalletRpcPlugin.ts

+   * @param param - See {@link AddEthereumChainRequest}
+   * @returns a Promise that resolves if the request is successful
+   */
+  public async addEthereumChain(param: AddEthereumChainRequest): Promise<void> {


I can see arguments for both approaches, but I think it's best to maintain consistency with the RPC method names.

danforbes · 2024-10-23T18:22:32Z

src/WalletRpcPlugin.ts

+type WalletRpcApi = {
+  wallet_addEthereumChain: (param: AddEthereumChainRequest) => void;
+  wallet_updateEthereumChain: (param: UpdateEthereumChainRequest) => void;
+  wallet_switchEthereumChain: (param: SwitchEthereumChainRequest) => void;
+  wallet_getOwnedAssets: (param: GetOwnedAssetsRequest) => GetOwnedAssetsResult;
+  wallet_watchAsset: (param: WatchAssetRequest) => boolean;
+};


Some minimal doc comments on these functions would be nice.

I actually still don't understand what is this for, I copied this approach from some rpc plugin example. It is used as Web3PluginBase<WalletRpcApi> but if I remove it, or change the names/types it doesn't affect anything in my plugin code. maybe @avkos @luu-alex @jdevcs could share some more. Anyway, it's internal use only.

src/WalletRpcPlugin.ts

danforbes · 2024-10-23T18:41:06Z

src/WalletRpcPlugin.ts

+   * @param param - See {@link UpdateEthereumChainRequest}
+   * @returns a Promise that resolves if the request is successful
+   */
+  public async updateEthereumChain(


The EIP for this method is stagnant and the method is not implemented by MetaMask. Are we sure we want to add this?

I will test all wallet rpc methods with a few different wallets and see what is actually implemented.

danforbes · 2024-10-23T18:42:32Z

src/types.ts

+    /**
+     * Array of asset interface identifiers such as ['ERC20', 'ERC721'].
+     */
+    types?: string[];


For some of these less straightforward optional parameters, it may be nice to document what the behavior is when the parameter is omitted.

The thing is, right now I don't know what happens and it's up to each wallet to decide how they handle that. I want to test it with a few popular wallets and see if there is any patter there. We can also introduce our own error types.

danforbes · 2024-10-23T18:44:08Z

src/types.ts

+  chainId: Numbers;
+  /**
+   * Asset interface ERC identifier, e.g., ERC20.
+   * Optional - EIP-1820 could be used.


What does this mean, exactly, that "EIP-1820 could be used"? If this property is omitted, how should the user interpret it?

I think I copied it from the EIP spec, I have no idea what does it mean :) I will remove it.

danforbes · 2024-10-23T18:47:03Z

test/wallet_addEthereumChain.test.ts

+    it("should return correct result", async () => {
+      const result = await web3.walletRpc.addEthereumChain({ chainId: 5000 });
+
+      expect(result).toBeUndefined();
+    });


What is the purpose of this test?

I don't know yet how to test actual wallet responses as each wallet can return whatever they want. I'm thinking to add some response parsers to always return the same type, despite wallet response, but I need to test it on real examples first (with real browser extension wallets). This test is not testing much yet, I agree.

…atchAsset (#4)

krzysu force-pushed the feat-example-app branch 2 times, most recently from 0e0fa84 to 338a5d4 Compare October 22, 2024 13:34

krzysu force-pushed the feat-more-methods-1 branch 3 times, most recently from 3456746 to 2764ecf Compare October 22, 2024 13:55

krzysu marked this pull request as ready for review October 22, 2024 13:56

krzysu requested a review from jdevcs October 22, 2024 13:56

jdevcs requested review from danforbes, avkos, Muhammad-Altabba and luu-alex October 23, 2024 08:45

jdevcs reviewed Oct 23, 2024

View reviewed changes

src/WalletRpcPlugin.ts Outdated Show resolved Hide resolved

jdevcs reviewed Oct 23, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

jdevcs reviewed Oct 23, 2024

View reviewed changes

krzysu commented Oct 23, 2024

View reviewed changes

danforbes approved these changes Oct 23, 2024

View reviewed changes

krzysu force-pushed the feat-example-app branch from 338a5d4 to 244812b Compare October 24, 2024 12:52

Base automatically changed from feat-example-app to main October 24, 2024 12:55

krzysu changed the title ~~feat: add updateEthereumChain, switchEthereumChain and getOwnedAssets~~ feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, watchAsset Oct 24, 2024

krzysu force-pushed the feat-more-methods-1 branch from 5ae1665 to b19b187 Compare October 24, 2024 13:27

feat: add more methods and types

93f518e

krzysu force-pushed the feat-more-methods-1 branch from b19b187 to 93f518e Compare October 24, 2024 13:28

improvements after code review

522e5b2

krzysu force-pushed the feat-more-methods-1 branch from c617bb1 to 522e5b2 Compare October 24, 2024 13:40

krzysu merged commit 44b6672 into main Oct 24, 2024
3 checks passed

krzysu deleted the feat-more-methods-1 branch October 24, 2024 13:42

github-actions bot mentioned this pull request Oct 24, 2024

chore(main): release 1.0.0 #7

Closed

krzysu added a commit that referenced this pull request Oct 24, 2024

feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, w…

fe633bf

…atchAsset (#4)

This was referenced Oct 31, 2024

chore(main): release 1.0.0 #17

Closed

chore(main): release 1.0.0 #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, watchAsset #4

feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, watchAsset #4

krzysu commented Oct 22, 2024

jdevcs left a comment

krzysu commented Oct 23, 2024

krzysu Oct 23, 2024

krzysu Oct 23, 2024

danforbes Oct 23, 2024

danforbes left a comment

danforbes Oct 23, 2024

danforbes Oct 23, 2024

danforbes Oct 23, 2024

danforbes Oct 23, 2024

krzysu Oct 24, 2024

danforbes Oct 23, 2024

krzysu Oct 24, 2024

danforbes Oct 23, 2024

krzysu Oct 24, 2024

danforbes Oct 23, 2024

krzysu Oct 24, 2024

danforbes Oct 23, 2024

krzysu Oct 24, 2024

feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, watchAsset #4

feat: add updateEthereumChain, switchEthereumChain, getOwnedAssets, watchAsset #4

Conversation

krzysu commented Oct 22, 2024

jdevcs left a comment

Choose a reason for hiding this comment

krzysu commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danforbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment