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

Mina: Add namespace / CAIP-2 #91

Merged
merged 14 commits into from
Mar 16, 2024
Merged

Mina: Add namespace / CAIP-2 #91

merged 14 commits into from
Mar 16, 2024

Conversation

halsaphi
Copy link
Contributor

This is the initial namespace specification / CAIP-2 for the Mina Protocol. The Mina Protocol is a L1 built from the ground up to use zero-knowledge proofs.

@halsaphi
Copy link
Contributor Author

halsaphi commented Sep 26, 2023

hi @bumblefudge is there anything else I need to do / add etc for this to be reviewed?

mina/caip2.md Outdated
- `devnet`
- `berkeley`

The human readable identifiers above may be composed to add description using `-` as a separator.
Copy link
Collaborator

@bumblefudge bumblefudge Sep 26, 2023

Choose a reason for hiding this comment

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

this is a little opaque-- are testnet-berkeley and testnet two different networks, or is the former a subset of the nodes of the latter that are running the berkeley nightly/feature-branch of the testnet codebase? i guess i'm confused what "compose" means when discussing network identifiers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, in this case maybe concatenated is better, will update ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes two different networks - at the moment Berkeley is a test network on which zkApps (smart contracts) are supported, (testnet-berkeley would be a testnet for the Berkeley test network - this doesn't exist but could if it was required) - to be merged to mainnet, testnet is the test network for the mainnet. Idea was to define an "all" encompassing naming convention that would also support any future networks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, ok. but is this convention already defined somewhere in the code or in the community? seeing that nodes return a networkID value implies a convention has already been set somewhere by whomever decides what a valid networkID is... is there a normative reference maybe? an MIP or even just a permanent URL in the docs?

Copy link
Contributor Author

@halsaphi halsaphi Sep 27, 2023

Choose a reason for hiding this comment

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

hi, by "this convention already defined" - do you mean berkeley/devnet/testnet etc names? Or do you mean concatenating testnet-berkeley - if we spun up a test network for the berkeley network? The names are embedded in the code ... the ability to concatenate is intended to "future proof" the convention in case we have more networks, it's new though we can add a definition of this to the docs and link it if it helps ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

heh, I guess what I mean by "convention definition" is that nothing internal (or native) to the namespace should be defined for the first time here at CASA, or canonically in a CASA spec-- the CASA spec is downstream of mina docs, the only thing being defined here is the export format for cross-chain purposes. so I guess what I mean is that if there are rules about what future network names will be valid (and instructions on how to embed those in code when you generate a new network/testnet/etc), linking to that from the references would be great! I highly recommend adding those kinds of definitions to the official docs sooner than later, because you never know who's tinkering a private repo with the expectation of being able to publicly add their private network to the list of known/public chainIds down the road...

mina/caip2.md Outdated

## Syntax

The blockchain namespace and blockchain identifiers will be in lowercase.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maximum length? whitespace allowed? other characters, diacritics, etc? a regex never hurts :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a regex ;D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @bumblefudge - added a regex and, hopefully ;), clarified what the possible names can be ... does it look ok now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is, I'll post a link to think PR in the mina discord so the mina community can comment

@bumblefudge
Copy link
Collaborator

hi @bumblefudge is there anything else I need to do / add etc for this to be reviewed?

Apologies, the queue has been a little backed up with travel and conferences! This looks fine except the nits mentioned above, can I get an independent review from a tooling company or experience Mina dev just in case I'm missing something out of ignorance?

Also, I noticed the Rosetta link in the references-- this is a cool open-source standards-support feature to highlight for cross-chain dapps and exchanges. One way of doing this would be to add that line to the references from the readme.md as well. Another would be to go ahead and write an CAIP-27 profile that gives dapps/exchanges all they need to send graphQL or rosetta queries to nodes, how to address/find archival nodes, etc. through walletConnect or other transport networks. Not blocking this PR or anything, just a suggestion if you want to get more crosschain interest organically through developer documentation :D

@halsaphi
Copy link
Contributor Author

@bumblefudge - thanks for the review! - will make some updates and come back to you ;D

Copy link
Collaborator

@bumblefudge bumblefudge left a comment

Choose a reason for hiding this comment

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

LGTM. if, down the road, the chainID naming convention gets formalized a bit more, remember to update this to point to whatever registry/IP-process/etc governs what names are valid and what names to be suspicious of!

@halsaphi
Copy link
Contributor Author

hi @bumblefudge - can this be merged? Or do I need to add more?

@bumblefudge
Copy link
Collaborator

hey sorry to do this to you but i dug into the docs again and noticed that the RPCs actually use a machine-readable hash to identify networks-- which is what i was asking about before, why invent a human-readable mapping that isn't canonically published anywhere?
https://docs.minaprotocol.com/exchange-operators/faq#why-am-i-getting-this-error-message-not-able-to-connect-to-the-network

@bumblefudge
Copy link
Collaborator

in other words, if "mainnet" is actually 5f704cc0c82e0ed70e873f0893d7e06f148524e3f0bdae2afb02e7819a0c24d1 why not just cut that down to the first 4 or 8 characters like most other CAIP-2 profiles? I keep thinking this human-readable indirection is a weak link/maintenance obligation and won't age well if people spin up their own private networks or testnets, i'm really leaning towards just doing a one-way transform (e.g. substr) of the canonical hash rather than adding the human-readable alias at the protocol level...

@halsaphi
Copy link
Contributor Author

halsaphi commented Dec 6, 2023

hi @bumblefudge, sorry missed this ;) This was discussed internally and the product team were keen to have a human readable name so the graphQL was extended to support this. Do you think we need to reappraise this or can we push forward with the proposal?

@bumblefudge
Copy link
Collaborator

bumblefudge commented Dec 6, 2023

interesting-- if human-readable is how the chainId system is actually working in the community's code "in the wild", then sure, the CASA scheme should reflect that. But my main concern is keeping CASA's docs maximally "downstream" of more canonical ones with lots of mina-community eyes on them and thus maximally low-maintenance. do you have a link to somewhere in the maintained docs about this mapping? When I google around, all I see are references to codename in the dockerized node setups, but what i'm looking for is the authoritative mapping in official docs or offical repos-- the CASA docs are NOT the place to define this archivally!

@ukstv
Copy link
Contributor

ukstv commented Dec 7, 2023

@halsaphi The next CASA call is on 14th of December. I guess, it makes sense for you to join there, so that we could have a chance to go through that change in a synchronous fashion, if @bumblefudge does not mind. Disclaimer: I like what Mina is doing.

@bumblefudge
Copy link
Collaborator

hey im not blocking the design I just want the upstream docs updated so I can link to them and know theyll be updated there when future mappings get added! attending an editorial meeting isn't a requirement but always welcome

@ukstv
Copy link
Contributor

ukstv commented Dec 7, 2023

"I just want the upstream docs updated so I can link to them" - yeah, I understand, and fully support the concern. Mina should have some sort of "Improvement Proposals" or RFC standardisation process on their own to specify available network names.

@bumblefudge
Copy link
Collaborator

if there isn't a page in the Mina docs and getting one added is out of your hands, just put in a comment like, "at time of writing the current recognized values are the ones listed above and no canonical human-readable or machine-readable mapping has yet been published. In future, the Mina official docs will link to such a mapping once one is published."

@halsaphi
Copy link
Contributor Author

hi @bumblefudge - sorry been busy at some events in Taipei - I'll add your comment to the spec and work on getting the mina docs updated ... once the docs contain the canonical mappings I'll update with a link to the docs.

@halsaphi
Copy link
Contributor Author

There is a update to the docs coming in Jan/Feb - will have the canonical mappings in that update and will then update the files here with links.

@halsaphi
Copy link
Contributor Author

halsaphi commented Mar 7, 2024

@bumblefudge @ukstv - I have added a link to some updated documentation showing the network identifiers and how to resolve the network identifier.

Can this be merged now? Is there anything I need to do for this?

Thanks

@halsaphi
Copy link
Contributor Author

halsaphi commented Mar 7, 2024

@bumblefudge and @ukstv - please note there is a planned upgrade to the Mina Networks (adding zkApps) - and that post this upgrade the documentation linked to above will move to https://docs.minaprotocol.com/ - I will update the link to reflect this post upgrade.

@ukstv
Copy link
Contributor

ukstv commented Mar 11, 2024

@bumblefudge This looks okay by me. What do you think?

@halsaphi
Copy link
Contributor Author

hi @bumblefudge and @ukstv - how do I get the second approving review?

@ukstv
Copy link
Contributor

ukstv commented Mar 15, 2024

@halsaphi I guess, the next step is to wait till the next CASA call. Usually, we merge PRs like this during our meeting. AFAIK, it is on April the 4th.

@bumblefudge
Copy link
Collaborator

bumblefudge commented Mar 15, 2024

@halsaphi I guess, the next step is to wait till the next CASA call. Usually, we merge PRs like this during our meeting. AFAIK, it is on April the 4th.

actually, you could be reviewer #2 today, @ukstv , and thus take one PR off the queue for the 4Apr21Mar meeting! :D

Copy link
Contributor

@ukstv ukstv left a comment

Choose a reason for hiding this comment

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

It's an honour to me!

Upd: Mea culpa. Removed "sirs" at the end, as it is gender exclusive.

@halsaphi
Copy link
Contributor Author

hi @bumblefudge and @ukstv thanks for all your help and patience with this ! :)

@bumblefudge bumblefudge merged commit 28f8b65 into ChainAgnostic:main Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants