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

CIP-0030 | Add chain id to differentiate between testnets #323

Closed

Conversation

TechRiderWR
Copy link

@TechRiderWR TechRiderWR commented Aug 25, 2022

With the current changes to the available environments and testnet networks (link), the network ID is no longer sufficient to differentiate between various testnets. Extension wallets should provide to the dapps the exact network they expect that the user is using.

@TechRiderWR TechRiderWR changed the title CIP-30: add network magic to differentiate between testnets CIP-30: Add network magic to differentiate between testnets Aug 25, 2022
@SebastienGllmt
Copy link
Contributor

I would slightly an endpoint that returns CIP34. That being said, I also prefer to add this kind of environment requirement into the enable endpoint rather than making it a getter such as #209 . Otherwise it's not always clear what the wallet UI should show

@rphair rphair changed the title CIP-30: Add network magic to differentiate between testnets CIP-0030 | Add network magic to differentiate between testnets Aug 26, 2022
@TechRiderWR TechRiderWR changed the title CIP-0030 | Add network magic to differentiate between testnets CIP-0030 | Add chain id to differentiate between testnets Aug 26, 2022
@TechRiderWR
Copy link
Author

@SebastienGllmt I've changed and included the request in the enable part. I left the getter in there as a convenience/debug function. I was considering also extending isEnabled, but there it made sense to me, that a single wallet should have a single session active at any time.

I've also added a supportedChains to the injected variables, which should allow better control on the dapp side.

@kevinhammond
Copy link
Contributor

This is a good idea of course. Thanks for taking it forward, @TechRiderWR !


chain_id =
{ 0 : bytes ; Name
1 : network_id ; NetworkId
Copy link
Contributor

Choose a reason for hiding this comment

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

This would still expect the regular 0 / 1 ? Should we state that?

Just because on the binary spec

When signing, it will still just expect to be just 0 / 1.

Copy link
Author

Choose a reason for hiding this comment

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

Not necessarily. I took this from the CIP-0034, where it's not 0/1 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a 4-bit number in the actual Cardano spec. The binary spec is arguably wrong here since although 0 & 1 are the only two values used now, larger values up to 15 are reserved for future use

@@ -164,11 +164,13 @@ type TxSignError = {

In order to initiate communication from webpages to a user's Cardano wallet, the wallet must provide the following javascript API to the webpage. A shared, namespaced `cardano` object must be injected into the page if it did not exist already. Each wallet implementing this standard must then create a field in this object with a name unique to each wallet containing a `wallet` object with the following methods. The API is split into two stages to maintain the user's privacy, as the user will have to consent to `cardano.{walletName}.enable()` in order for the dApp to read any information pertaining to the user's wallet with `{walletName}` corresponding to the wallet's namespaced name of its choice.

### cardano.{walletName}.enable(): Promise\<API>
### cardano.{walletName}.enable(chainId: cbor\<chain_id> = undefined): Promise\<API>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of taking a position parameter, I think it's better if we make the enable function take in an object

{
    chainId?: cbor\<chain_id>
}

This makes it easier to add more options later since working with optional positional arguments gets messy

Copy link
Author

Choose a reason for hiding this comment

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

Very true. I've corrected it. Do you see currently any more options that would impact the dapp <> wallet communication.

@TechRiderWR
Copy link
Author

TechRiderWR commented Aug 28, 2022

@KtorZ @SebastienGllmt I saw #208 PR. I understand the sentiment to try to treat this API as something static. There doesn't seem to be a clear idea around "structured process for bringing extensions to this API" without a clear owner. With wallets bringing their own spin on some of the unclear endpoints (balance vs utxos on Nami or Eternl, Eternl differentiating between locked/unlocked utxos and balance, every wallet handling collaterals differently; change address differences; no way to clearly identify a wallet as all wallets have different interpretations for address semantics - e.g. getting the first address to have a good UX that is used on other chains) this CIP-30 is getting out of date very quickly.

With the the changes around collaterals after Vasil, with the different environments I'd argue that it's already out of date the moment Vasil goes live. You guys were the last to add some changes to this API. All of the changes above are backwards compatible. I'm not familiar with the policy how CIPs would be deprecated. Unless you guys see a need to create a completely new CIP with a new API, I'd keep this CIP updated. Splitting some of the new functionality into a new CIP would only cause confusion as it'd be harder to gather what would need to be supported by wallets, if there is no single source of truth.

If we are talking extendibility. I can extend the CIP enable function to get a second option - version and we can mark each function the API version they were added and at which point there were deprecated. Alternatively to not overload wallets to handle the version logic, we can add an apiVersion to the injected and it would be up to the dapps to decide if they allow connecting to said wallets.

What do you guys think?

I'm relatively new to CIPs on cardano, what would be the steps to set up an actual live standard otherwise?

@rphair
Copy link
Collaborator

rphair commented Aug 28, 2022

(comment withdrawn; had been based on misunderstanding)

@dcoutts
Copy link
Contributor

dcoutts commented Aug 30, 2022

If what you want is to distinguish between networks (including different testnets) and it's just for prevention of accidents (i.e. no real security concerns) then what you want is just the "network magic" (32bit) since that's different for each network.

IMHO, it'd be preferable to reuse existing concepts (the network magic) rather than inventing new ones (a "chain id").

Alternatively, if this has some security issue then it might be better to use the genesis hash. But if so, there's no need for all the other things in this "chain id". The proposed chain id looks like overkill, and introducing new unnecessary concepts.

3 : $hash32 ; GenesisHash
}

network_id = uint .le 16
Copy link
Contributor

Choose a reason for hiding this comment

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

should be lt and then le since 16 is not included

@ehanoc
Copy link
Contributor

ehanoc commented Aug 31, 2022

I personally don't like the idea of the dApp enabling / controlling the wallet's API to use a specific network. Wouldn't it make more sense for the network choice to a conscious decision of the user within the wallet's itself. The wallet remains in full control and we don't have to handle all the different prompts (permissions), errors cases (good network, wrong network, unavailable network) that could potentially introduce unexpected outcomes.

With getNetworkId (already in spec); the dApp can notify the user that is using the wrong network and it should change in the wallet itself?


```
type EnableOptions = {|
chainId?: cbor\<chain_id>

Choose a reason for hiding this comment

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

Why use cbor here? I think we should be using dev/human-friendly types when possible (as with address that supports bech32 string in cip30).

I suggest to either change this to the "human-readable string" format that is already described in cip34, or make it support both formats.

@@ -202,6 +218,12 @@ Errors: `APIError`

Returns the network id of the currently connected account. 0 is testnet and 1 is mainnet but other networks can possibly be returned by wallets. Those other network ID values are not governed by this document. This result will stay the same unless the connected account has changed.

### api.getChainId(): Promise\<chain_id>
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency with the supportedChains API property, I guess this should rather be

Suggested change
### api.getChainId(): Promise\<chain_id>
### api.getChainId(): Promise\<cbor\<chain_id>>

@@ -189,6 +201,10 @@ A name for the wallet which can be used inside of the dApp for the purpose of as

A URI image (e.g. data URI base64 or other) for img src for the wallet which can be used inside of the dApp for the purpose of asking the user which wallet they would like to connect with.

### cardano.{walletName}.supportedChains: cbor\<chain_id>[]
Copy link
Contributor

@refi93 refi93 Sep 1, 2022

Choose a reason for hiding this comment

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

Suggested change
### cardano.{walletName}.supportedChains: cbor\<chain_id>[]
### cardano.{walletName}.getSupportedChains(): Promise<\cbor\<chain_id>[]>

I would suggest for the sake of consistency with the rest of the API to make this an async method.

Moreover, depending on the implementation of the connector on the wallet (extension) side, it may be an unnecessary complication to make this only method/property return the result synchronously, especially if proxying through multiple layers (content script, background worker, ...) is involved

@szist
Copy link

szist commented Sep 5, 2022

With getNetworkId (already in spec); the dApp can notify the user that is using the wrong network and it should change in the wallet itself?

getNetworkId is not enough as described in the comments. There is no way to differentiate between testnets currently.

@TechRiderWR
Copy link
Author

If what you want is to distinguish between networks (including different testnets) and it's just for prevention of accidents (i.e. no real security concerns) then what you want is just the "network magic" (32bit) since that's different for each network.

IMHO, it'd be preferable to reuse existing concepts (the network magic) rather than inventing new ones (a "chain id").

Alternatively, if this has some security issue then it might be better to use the genesis hash. But if so, there's no need for all the other things in this "chain id". The proposed chain id looks like overkill, and introducing new unnecessary concepts.

That's what I've added originally, based on @SebastienGllmt suggestion I changed it to chain_id. Since there is no governance structure set up I don't know which way I should change things.

@SebastienGllmt
Copy link
Contributor

SebastienGllmt commented Sep 6, 2022

IMHO, it'd be preferable to reuse existing concepts (the network magic) rather than inventing new ones (a "chain id").

The protocol already made the decision of having two different numbers per network. If the wallet has access to both network magic and the network ID, it can service a decent number of the CIP30 endpoints without having a server for that network. If we decide this isn't something we care about, we can decide to only use the network magic

@rphair
Copy link
Collaborator

rphair commented Sep 13, 2022

The CIP Editors' Meeting today noted that we are still trying to get input from more developers about whether it would be best to use the network magic or the network magic+ID. The current status is that this question should be decided before merging this CIP.

I was thinking we might use the IOG Dev Digest to poll on this question & direct devs here for their input, but the last issue was on 02 September so the next issue won't be in time for the next meeting. If we still haven't settled this question here in GitHub by the next CIP meeting in 2 weeks, we could then ask IOG Marketing to include it in the Dev Digest.

@KtorZ KtorZ added the Update Adds content or significantly reworks an existing proposal label Sep 27, 2022
@coot
Copy link

coot commented Sep 27, 2022

It's not really clear to me why do we need chain id while we already have network magic for exactly the purpose of differentiating networks. We can have different 2^32 networks, we are not any near that number, don't we?

If we want an association between official networks and their official string identifier, I'd rather see a json file maintained by, let say CF, with a formal process of identifying official networks. This could then be used by wallets, or nodes to make logs / user messages nicer. Although the drawback is that one could false the user its using a different network than she/he expects. So maybe not a great idea after all.

Also note that information exchanged in the handshake should be bounded, so the handshake message fits into a single packet < 1460 bytes). There's still a lot of space, but the future might bring more interesting applications.

chainId?: cbor\<chain_id>
|}
```
Options passed to the `enable` function. The optinal `chainId` should be encoded as cbor according to the schema defined in CIP-0034.
Copy link

Choose a reason for hiding this comment

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

spelling mistake:

Suggested change
Options passed to the `enable` function. The optinal `chainId` should be encoded as cbor according to the schema defined in CIP-0034.
Options passed to the `enable` function. The optional `chainId` should be encoded as cbor according to the schema defined in CIP-0034.

@KtorZ KtorZ added the Category: Wallets Proposals belonging to the 'Wallets' category. label Mar 18, 2023
@Crypto2099
Copy link
Collaborator

It seems this pull request already aims to address part of the issue that I added in this issue I created: #489

In a side/multi-chain future where Cardano-native sidechains will exist, I do not think it is enough to rely on just getNetworkId() or the network magic alone to differentiate which chain you are connecting to.

The currently existing getNetworkId() is good as a binary true/false for determining whether the wallet is connected to a test or main network. Adding a new, human-readable and developer friendly syntax to the API to further differentiate which network is being connected to is beneficial IMO.

@Crypto2099
Copy link
Collaborator

Why the requirement to pass and receive these arguments/responses in CBOR syntax?

This presents additional downstream requirements on libraries such as the CSL/CML/etc to implement encoding and decoding features before this feature can be rolled out and implemented ultimately in developer-friendly JavaScript object syntax anyway.

@Ryun1
Copy link
Collaborator

Ryun1 commented Jun 20, 2023

Hey @TechRiderWR ,

This proposal was discussed today in CIP editors call 68. Are you still interested in pursing this amendment or perhaps there has been a work around discovered? because as it stands I do not think it is advisable to continue to alter CIP-30. Rather I would strongly recommend any more alterations to CIP-30 utilize the extensibility mechanism introduced in #446. This is to avoid the past issues of versioning of CIP-30 for both dApps and wallets, see #446 for discussions.

If this is still an issue with CIP-30 (I believe so as #209 and issue #489 also are similar) I would suggest packaging this in with other CIP-30 amendments (such as #443 and/or maybe #206) to create a multi-functional CIP-30 extension. I mentioned this idea in this issue comment. Would you be willing to pursue something like this?

@Ryun1
Copy link
Collaborator

Ryun1 commented Mar 10, 2024

Due to little activity on this proposal and the current state of CIP-30 extensions, I think we can move to close this one @rphair @Crypto2099

@rphair
Copy link
Collaborator

rphair commented Mar 11, 2024

@TechRiderWR I concur about closing this due to apparent abandonment, which makes a quorum of currently active editors. We seem to be marching along well with the testnets as they are currently defined. But if anyone (author or community) has current grounds for reopening this, please post here and we can do so after the PR is properly rebased on the master branch.

@rphair rphair closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category. Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.