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

Add documentation for the Indexed Merkle Map API #1088

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Shigoto-dev19
Copy link
Contributor

No description provided.

@Shigoto-dev19 Shigoto-dev19 requested a review from a team as a code owner January 10, 2025 20:17
Copy link

vercel bot commented Jan 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 8:20pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
07-oracles ⬜️ Ignored (Inspect) Jan 10, 2025 8:20pm

Copy link
Contributor

@45930 45930 left a comment

Choose a reason for hiding this comment

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

I think it's good to have this article alongside the typedoc, but I'd personally prefer to highlight ZkProgram rather than smart contract.

I'm also open to just promoting the API out of experimental, especially with this article written. Let's see what @Trivo25 thinks 🤔 , so if we do that, we can also remove some of the Experimental references here.

This is possible because the `IndexedMerkleMap` can be used within provable code.

```ts
class MyContract extends SmartContract {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather write new examples using zkProgram since we have more confidence that it will not be changed soon. SmartContract is more likely to be modified by the new API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regardless of documentation maintainability, I think that demonstrating a SmartContract example for the Indexed Merkle Map API provides greater benefits for developers and serves as more effective documentation.

When someone wants to use the Indexed Merkle Map API, they will likely integrate it directly within a smart contract, as it works seamlessly in provable code.

In contrast, a ZkProgram example might lead readers to mistakenly believe that the API should be used with a ZkProgram. This misunderstanding could imply that verifying a zkProgram proof for Indexed Merkle Map updates is straightforward, when in fact it is only practical in advanced scenarios such as recursive reducers.

Copy link
Contributor

Choose a reason for hiding this comment

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

This misunderstanding could imply that verifying a zkProgram proof for Indexed Merkle Map updates is straightforward, when in fact it is only practical in advanced scenarios such as recursive reducers.

Maybe I don't understand this myself. What's wrong with this example:

import { Experimental, Field, SelfProof, ZkProgram } from 'o1js';

const height = 5;
class MyMerkleMap extends Experimental.IndexedMerkleMap(height) {}

const RecursiveProgram = ZkProgram({
  name: 'RecursiveProgram',
  publicOutput: MyMerkleMap,
  methods: {
    init: {
      privateInputs: [],
      method: async () => {
        const map = new MyMerkleMap();
        return { publicOutput: map };
      },
    },
    insert: {
      privateInputs: [SelfProof, Field, Field],
      method: async (
        proof: SelfProof<undefined, MyMerkleMap>,
        key: Field,
        value: Field
      ) => {
        proof.verify();
        const newMap = proof.publicOutput.clone();
        newMap.insert(key, value);
        return { publicOutput: newMap };
      },
    },
    update: {
      privateInputs: [SelfProof, Field, Field],
      method: async (
        proof: SelfProof<undefined, MyMerkleMap>,
        key: Field,
        value: Field
      ) => {
        proof.verify();
        const newMap = proof.publicOutput.clone();
        newMap.update(key, value);
        return { publicOutput: newMap };
      },
    },
  },
});

await RecursiveProgram.compile();

class RecursiveProof extends ZkProgram.Proof(RecursiveProgram) {}

const p = await RecursiveProgram.init();
const p2 = await RecursiveProgram.insert(p.proof, Field(1), Field(2));
const p3 = await RecursiveProgram.update(p2.proof, Field(1), Field(3));

console.log(p3.proof.publicOutput.get(Field(1))); // 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's nothing wrong with the example but I think whoever wants to use the Indexed MM would use it directly within a zkApp as it's more practical and for that, I think a SmartContract example would be a better guide for the reader and less confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think whoever wants to use the Indexed MM would use it directly within a zkApp as it's more practical

I'm not sold on this. Do you actually believe indexed merkle map is a usable solution for zkapps today? To me, your example is misleading because it teases the existence of a map API that works on Mina, but doesn't address the same old concurrent update or state storage issues that exist for every zkapp. In production, this example would not work with multiple users accessing it at once, and I want to stop creating new examples that break down at scale.

I know that this is a touchy issue in the community right now as protokit devs are looking for L1 alternatives to the StateMap so that they can deploy on mainnet: (discord example). IMO there is absolutely no alternative that currently works, and I don't want to beat around the bush with that fact and pretend this is a viable alternative. Do you disagree with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I don't disagree!

I see your point here! But that's also applicable to many primitives in the documentation. I was trying to keep it consistent with the current o1js docs as I was documenting the Indexed Merkle Map API.

Copy link
Contributor

Choose a reason for hiding this comment

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

My goal with example code in the docs is for developers to grasp the concept and then take it to build one step further. My problem with the pattern we have in most of our existing documentation is that it only works to a certain point, and that's what we show. When someone tries to take the next step, they hit a wall.

What I like about ZkProgram is that the final product can be a Mina ZkApp, or it could live on protokit or zeko. And before deploying it to any network, a developer can change different variables, combine it with other ZkPrograms, and extend it in all kinds of ways locally.

console.log(indexedMerkleMap.data.get().sortedLeaves);
```

## Indexed Merkle Map - API reference
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel strongly that this should not be included here. This definitely belongs in the Typedoc reference, so we should work on generating that correctly instead of manually adding auto-generated type declarations here.

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.

2 participants