Report of WalletAccounts tests

[PhilippeR26]

Context

I have ended the tests of WalletAccount .
I have some comments :

• Both in wallet.ts and account.ts, there are methods to subscribe to an event, but there is nothing to unsubscribe.
• 2 events are subscribed in the constructor of WalletAccount class, but as there is no destructor in Typescript, what are the consequences for the DAPP and for the wallet to create / get out of scope many times WalletAccount instances?
• Starknet.js is supposed to simplify the work of the devs, but I consider that the methods about events are not really useful, as it's easier to handle events directly with the swo (StarknetWindowObject) :
  With Starknet.js :

myAccountWallet?.onAccountChange(handleAccount);
// or
if (!!swo) {
    wallet.onAccountChange(swo, handleAccount);
}

Directly with the swo :

swo?.on("accountsChanged", handleAccount);
// then
swo?.off("accountsChanged", handleAccount);

About automatic change of account, the re-initialization of cairoVersion is missing here

starknet.js/src/wallet/account.ts

Line 55 in 4b83943

// Update Address on change

.
About automatic change of chain, the change of this.chainId without changing the nodeurl is generating a complete mess in the DAPP. I think that if the DAPP accepts to change of chain, it has to propose a Rpc provider for each chain accepted. So, in this spirit, it will be the responsibility of the DAPP to convert a new chainId to a new Rpc provider. I think it will be important in the case of layer 3. So, we could delete setChainId, and replace it by something accepting the RpcProviderOptions as input.

Also one little thing, about the access to the frontend provider : if the DAPP wants to do something with the provider in use in the Wallet Account, it needs :

const txR = await myWalletAccount?.channel.waitForTransaction(txH);

Normally, channel should be hidden to the DAPP devs. Couldn't we have something like :

const txR = await myWalletAccount?.provider.waitForTransaction(txH);

Desktop (please complete the following information):

• Browser & version [e.g. chrome, safari, webworker] : Chrome v123.0.6312.86
• Node version [e.g. 16.0.1] v21.7.1
• Starknet.js version : last next-version commit
• Network [devnet, testnet] : Sepolia/mainnet
• Wallets : ArgentX v5.13.5 (22/feb/2024, ID :pgicobkbegddeldhlnnacicgioehkbem ), Braavos 0.0.2 experimental

Additional context
Add any other context about the problem here.

[EjembiEmmanuel]

@PhilippeR26 I'll like to work on this

[PhilippeR26]

Hello,
I think it's already handled by core team (@ivpavici )

[PhilippeR26]

@tabaktoni
I have a strange error when using WalletAccount.deployAccount() :

Error: Expected valid bigint: 0 < bigint < curve.n

I think that you should override deployAccount method, using UDC.

[PhilippeR26]

Following the last commits of the Wallet API, 2 things have to be implemented :

• new wallet_supportedWalletApi entry point
• optional parameter api_version added in most of entry point.
• We should also consider to have Starknet.js compatible with several Wallet API versions (as for starknet rpc API).

Warning

Currently not open to contributors.

[ivpavici]

@PhilippeR26 can we close this?

[PhilippeR26]

If you consider that all identified problems are solved, or are minor, or will be solved later, you can close.
Personally, I think that @tabaktoni should dig a little in these subjects.