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 tutorial 5 to tutorial runner #577

Closed
wants to merge 5 commits into from
Closed

Conversation

Trivo25
Copy link
Member

@Trivo25 Trivo25 commented Sep 5, 2023

closes #514

@Trivo25 Trivo25 requested a review from a team as a code owner September 5, 2023 12:47
@vercel
Copy link

vercel bot commented Sep 5, 2023

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 Nov 22, 2023 8:12am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
07-oracles ⬜️ Ignored (Inspect) Visit Preview Nov 22, 2023 8:12am

Copy link
Contributor

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

I have a few questions, most importantly about skipping the last part of the tutorial.

Pre-approving!


```ts src/run.ts
3 const num1 = UInt32.from(40);
4 const num2 = UInt64.from(40);
Copy link
Contributor

Choose a reason for hiding this comment

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

maintaining these line numbers going forward seems tedious to me :/

maybe there could be a default where it just appends to the file when there are no line numbers? (it'll still be tedious but at least only when the line numbers are needed)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is very tedious :( We would have to make some adjustments to the tutorial runner but possible (and probably worth it)

Copy link
Contributor

Choose a reason for hiding this comment

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

YAY no more line number requirement! Let's omit the tedious line numbers per convo at offsite

@@ -294,123 +337,126 @@ If a program is too large to fit into these constraints, it can be broken up int

You can use [Merkle trees](../snarkyjs-reference/classes/MerkleTree) to manage large amounts of data within a circuit. The power of Merkle trees is demonstrated in the [05-common-types-and-functions/src](https://github.com/o1-labs/docs2/tree/main/examples/zkapps/05-common-types-and-functions/src) reference project for this tutorial. See the [BasicMerkleTreeContract.ts](https://github.com/o1-labs/docs2/blob/main/examples/zkapps/05-common-types-and-functions/src/BasicMerkleTreeContract.ts) contract and [main.ts](https://github.com/o1-labs/docs2/blob/main/examples/zkapps/05-common-types-and-functions/src/main.ts) that demonstrates how contracts interact with Merkle trees and how to construct them.

Copy link
Contributor

Choose a reason for hiding this comment

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

well.. no big deal to delete, but you could leave it in there if you used an ignore directive like for skipped commands. Do we have that?
EDIT: nvm I saw that you used it elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we do have ignore. I can also add it back, I don't mind. I just felt like it was odd that we only mentioned the import for MerkleTree, but didn't talk about importing all the other previous primitives. So rather than adding an import to all the previous sections I removed this one 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!

this.treeRoot.set(rootAfter);
}
```ts src/run.ts
117 class BasicMerkleTreeContract 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.

hmm, so having the dots ... as a way to tell the tutorial runner "skip any number of lines" would actually be useful. doesn't have to be in this PR but the sooner the better before we have to change all tutorials just because we don't have this feature

const value = Field(50);

map.set(key, value);
```ts ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

is ignoring all the MerkleMap code intended? ideally we also test that part

@barriebyron
Copy link
Contributor

@Trivo25 I lost track of the status of this PR... were we waiting to merge it until the dependency on line numbers in the tutorial runner is removed? If we are waiting, wdyt we can move the PR to draft?

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.

Add Tutorial 5: Common Types and Functions to automated tutorial runner
4 participants