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

docs: Onchain notes tutorial #604

Merged
merged 14 commits into from
Apr 26, 2024
Merged

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Apr 12, 2024

No description provided.

Copy link
Contributor

@mFragaBA mFragaBA left a comment

Choose a reason for hiding this comment

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

Looks Good! I think that since the init command will be introduced in the upcoming release of miden-client, we can update the getting-started tutorials in order to do the setup with:

  1. cargo install ...
  2. miden-client init (and overriding the necessary configurations)

@mFragaBA
Copy link
Contributor

mFragaBA commented Apr 12, 2024

I tried following the getting-started steps but got stuck when trying to import the first input note:

FungibleAssetInvalidWord([7023146166627099489, 7078034031914609772, 7596728195356390241, 8390045827181995364])

I'm guessing it's because the deployed faucet at https://ethdenver.polygonmiden.io/ has an incompatible version for the current client. Is it possible to deploy a new version @phklive? I can still replicate it by hand but it would be nice to be able to test the actual flow.

Edit: upon second thought, you'll probably need a release of the client before deploying the faucet

Copy link
Contributor

@mFragaBA mFragaBA left a comment

Choose a reason for hiding this comment

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

Looks good, I think with the miden-client init inclusion it should be good to merge

Copy link
Collaborator

@Dominik1999 Dominik1999 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the updated version. I will do another review when we have our remote nodes running. Then I can test the tutorial against a real node.

docs/introduction/get-started/p2p-private-offchain-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-private-offchain-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Dominik1999 Dominik1999 left a comment

Choose a reason for hiding this comment

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

Love it, we only need to wait for the new faucet address before we can merge

docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
docs/introduction/get-started/p2p-public-note-txs.md Outdated Show resolved Hide resolved
@kmurphypolygon
Copy link
Contributor

@Dominik1999 @bobbinth @mFragaBA @igamigo I'll check this again when the faucet works and I can update the preliminary step too. Let me know.

A few language tips: avoid the future tense and the passive tense in favor of present simple. Avoid the word "utilize", just say "use". Use US English.

Thank you :)

Copy link
Contributor

@kmurphypolygon kmurphypolygon left a comment

Choose a reason for hiding this comment

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

Updated the language, added some clarifications, tested the flow. 100% happy everything is working now.

@Dominik1999
Copy link
Collaborator

Ok, then we rebase and merge to main

@bobbinth bobbinth changed the base branch from next to main April 26, 2024 09:00
@Dominik1999 Dominik1999 merged commit 48d0a34 into main Apr 26, 2024
9 checks passed
@Dominik1999 Dominik1999 deleted the igamigo-onchain-notes-tutorial branch April 26, 2024 09:38
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.

5 participants