KIP-0037 to KIP-0041: Standardized JSON-RPC Methods for Wallet-DApp Communication #71
Conversation
…pdated kips for wallet-adapter
nillo
changed the title
|
Can't we just create on KIP for all functions? |
… kadena_disconnect_v1 and kadena_getActiveAccount_v1 APIs
barthuijgen
changed the title
There was a problem hiding this comment.
Took a look at KIP 37 and KIP 38 (will get to the rest later). It looks good so far, but I did have some questions and rewording recommendations (see the comments for details).
The main question I'd like to understand better before approving is the following:
- Is an account name always associated with a contract on the wallet side? Particularly, non-minted accounts?
|
|
||
| ## Security Considerations | ||
|
|
||
| - **Data Accuracy:** Ensure that the returned account information is accurate |
There was a problem hiding this comment.
Could the proposal specify who needs to "Ensure that the returned account information is accurate".
There was a problem hiding this comment.
The KIP is written for wallets to implement the RPC methods, so the remarks is for the creator of the wallets, i could add, "The wallet creator implementing the RPC methods.." Ensure that the returned account information is accurate before it but that seems kind of redundant to me.
| keys: string[]; // Array of public keys (secret keys are omitted for security). | ||
| pred: string; // Predicate defining key validation (e.g., "keys-all", "keys-any"). | ||
| }; | ||
| chainAccounts: string[]; // Array of chain IDs where this account exists |
There was a problem hiding this comment.
chainAccounts should be reworded to better reflect that it represents the chains the account exists in.
Maybe activeChains or availableChains?
There was a problem hiding this comment.
@barthuijgen @alber70g, i can live with this update thoughts?
There was a problem hiding this comment.
Between the two I think I like availableChains the best. chainAccounts used to represent something else before, now it indeed does not make much sense.
There was a problem hiding this comment.
What is the benefit of having chainIds in general?
This will force wallets to query all chains, which makes the response time longer (and also more error-prone) even if the wallet stores data still need to sync data since account creation might happen outside of the wallet with transferCreate.
Also, I can imagine that wallets might just send an empty array to avoid calling all chains, which would then make the response unpredictable and untrustworthy to the end user.
the user can use accountDiscovery from client-utils to fetch not only chainIds but also the account balance. or if we want the provider returns that we can add another message like kadena_discoverAccount_v1.
| pred: string; // Predicate defining key validation (e.g., "keys-all", "keys-any"). | ||
| }; | ||
| chainAccounts: string[]; // Array of chain IDs where this account exists | ||
| } |
There was a problem hiding this comment.
Would y'all think its useful to allow for an optional field for an account label, assuming that the wallet allows for user-picked labels for their accounts. For example:
interface AccountInfo {
...
chainAccounts: string[];
label?: string; // Example: "My Main Account 1"
}There was a problem hiding this comment.
I don't think this is a assumption that should be made, its either there or its not, if we want it to be there we should enforce it having that said. I seems like a UI thingy to me, the wallet provides the account <- the Dapp itself could provide the ability to add a label to it. Currently none of the wallets support this, so i don't think there is a necessity for this, and should leave labeling to the dapp itself. @barthuijgen @alber70g thoughts?
There was a problem hiding this comment.
Chainweaver v2 and v3 both have support for aliasses though. If a user is actively using aliasses, it would be nice to have it. I would suggest not to have it nullable, but empty string if it's not there. However current APIs don't support it, correct?
There was a problem hiding this comment.
Our MetaMask Snap also supports an alias, and does expose it via the API. If it's just optional I don't think it would hurt to have this. It would be difficult to get the information any other way.
Empty string seems weird to me though. I think it should be label: string | null or label?: string.
There was a problem hiding this comment.
Agreed. label? seems reasonable
| interface AccountInfo { | ||
| accountName: string; | ||
| networkId: string; | ||
| contract: string; |
There was a problem hiding this comment.
(This is related to the comment related to contract I posed in KIP 37):
And is it possible for the same accountName to be present in multiple contracts? If yes, would there be multiple entries per contract?
There was a problem hiding this comment.
Yes, when accountName is used for multiple contracts, it's passed multiple times.
@nillo @barthuijgen we can add a note to indicate this to clarify (if it's not already there)?
| The method **MUST** return an array of account objects, even if the wallet only | ||
| supports a single account. | ||
|
|
||
| Note that the response can have multiple accounts with the same `accountName`. |
There was a problem hiding this comment.
I know the examples section has an example that relates to this (Single Account, Multiple Guards), but I think it would be good to elaborate on the circumstances that could result in multiple entries with the same account name. It could be included here or in the Behavior Requirements section.
kip-0038.md
Outdated
| supports a single account. | ||
|
|
||
| Note that the response can have multiple accounts with the same `accountName`. | ||
| The `guard` is what makes each unique. This allows for dapps to use non-minted |
There was a problem hiding this comment.
The
guardis what makes each unique.
Is this statement accurate?
There could be an account test1 governed by guardA and also an account test2 that is also governed by guardA.
This could happen when working with vanity accounts or accounts associated with contracts that don't enforce principaled (e.g. that account name must correspond to its associated guard) account names.
There was a problem hiding this comment.
In that case they don't have the same accountName, right?
Maybe the wording could be changed
- The `guard` is what makes each unique. This allows for dapps to use non-minted
+ The `guard` is what makes accounts with the same `accountName` unique. [...]| accountName: string; // The unique identifier for the account. | ||
| networkId: string; // The unique identifier for the network for this account. | ||
| contract: string; // Identifier for the fungible token contract. | ||
| guard: { |
There was a problem hiding this comment.
To get ahead of potential confusion over the definition of "guard" (https://docs.kadena.io/smart-contracts/guards) here:
It might be good to add a note somewhere in the document that we're using the term "guard" here but it focuses mainly on keyset-related guards (keyset ref, single key, multi-key, and w accounts) since that the current ownership model that wallets support.
There was a problem hiding this comment.
It's any guard honestly. We should maybe extend docs to include all types of guard?
https://gist.github.com/jmcardon/4290533dc950fe49ebe5c4feee342ba0
There was a problem hiding this comment.
I think guard here makes sense, the other place you mentioned should be changed to refer to multiple keys indeed, which is still one guard.
There was a problem hiding this comment.
@alber70g it can't be smart contract related guards since you cant add them to your wallet directly. the valid guards are Keyset and Keyset ref. the current api only support keyset - {keys: string[]; pred: string} . its good to mention it in the KIP and make the guard type more future-proof, allowing support for Keyset Ref later without breaking the API.
There was a problem hiding this comment.
| interface AccountInfo { | ||
| accountName: string; // The unique identifier for the account. | ||
| networkId: string; // The unique identifier for the network for this account. | ||
| contract: string; // Identifier for the fungible token contract. |
There was a problem hiding this comment.
Should contract be optional for accounts that aren't minted yet? Or do wallets always associate accounts with a contract right away?
Also, is it possible for the same accountName to be present in multiple contracts? If so, should this field be a list similar to chainAccounts?
There was a problem hiding this comment.
Wallets SHOULD associate new accounts with a contract. If they don't we cannot accept it as an account. An account exists in the context of an implementation of a fungible-v2 interface.
There was a problem hiding this comment.
Also, is it possible for the same accountName to be present in multiple contracts? If so, should this field be a list similar to chainAccounts?
The wallet only returns the ACTIVE account. Since an account is always tied to a contract, this is not a list
|
One thing I'd consider as this will become confusing. Instead of kadena_getAccounts_v2 API and kadena_getAccounts_v1 API lets not have multiple versions as that would imply updated API features no? |
Kadena Improvement Proposals (KIPs) for Wallet RPC Standardization
Summary
This pull request introduces a set of Kadena Improvement Proposals (KIPs) that define a standardized JSON-RPC interface for decentralized application (dApp) and wallet communication. The proposed RPC methods will enable a more unified and developer-friendly approach to wallet integration across the Kadena ecosystem.
Motivation
Currently, wallet integrations in Kadena's ecosystem vary widely, with different wallets exposing their own methods via browser extensions, HTTP APIs, or WalletConnect. While KIP-0017 provides a standard for WalletConnect-based communication, there is a need for a more general and extensible RPC framework that can support:
Included KIPs
This PR introduces the following KIPs:
kadena_getAccount_v1APIkadena_getAccounts_v2APIkadena_getNetwork_v1APIkadena_getNetworks_v1APIkadena_changeNetwork_v1APICompatibility & Migration
These proposals do not replace KIP-0017 but extend it with additional RPC methods that can be adopted by wallets. Existing integrations using WalletConnect will continue to function as expected, with wallets able to gradually adopt the new standardized methods.