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

feat: add creation of onchain notes #587

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

mFragaBA
Copy link
Contributor

@mFragaBA mFragaBA commented Apr 9, 2024

Updates create_basic_fungible_faucet and create_basic_wallet to take an extra parameter that determines the account storage type (i.e. whether the account is stored onchain or offchain).

This is needed to create onchain stored accounts on miden client.

related to #171 from miden-client

@bobbinth
Copy link
Contributor

bobbinth commented Apr 9, 2024

Is this ready for review?

@mFragaBA mFragaBA marked this pull request as ready for review April 9, 2024 19:32
@mFragaBA
Copy link
Contributor Author

mFragaBA commented Apr 9, 2024

Is this ready for review?

yea, I was waiting for CI to pass 😅. Also I mentioned on discord an issue I've had when testing this change on miden client. I'm getting an account hash mismatch error when I try to run a mint transaction against an onchain faucet. Perhaps I'm missing something else here in miden-base.

Building proven transaction error: Proven transaction account_final_hash 0x45c9f037a4a14b82f22811c922be778b67269165c9f1dc88552e10406fec9379 and account_details.hash must match 0x5b6dd5c0bec82c82d9f3ee0417fc15cc86f9e032aa3c2852946da05712e2b6c7.

more details here

@bobbinth
Copy link
Contributor

bobbinth commented Apr 9, 2024

Also I mentioned on discord an issue I've had when testing this change on miden client. I'm getting an account hash mismatch error when I try to run a mint transaction against an onchain faucet. Perhaps I'm missing something else here in miden-base.

Is the on-chain faucet "new" (i.e., was created not during genesis but by creating a new account). If so, the issue may be related to #529 (comment).

Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

Not related to this PR: account_type and account_storage_type seems too similar. Maybe we should rename account_storage_type into account_storage_mode.

Comment on lines 38 to 39
auth_scheme: AuthScheme,
account_storage_type: AccountStorageType,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: I would probably move account_storage_type up to be before auth_scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed!

@mFragaBA
Copy link
Contributor Author

mFragaBA commented Apr 9, 2024

Is the on-chain faucet "new" (i.e., was created not during genesis but by creating a new account). If so, the issue may be related to #529 (comment).

It is indeed! I'm now trying to replicate the changes on that PR and see if it gets fixed

UPDATE: tried cherry picking the first two commits from that PR (assuming that's where the fix was and later on refactors took place) and I'm still getting the same error

@igamigo igamigo merged commit 8e14ec4 into next Apr 9, 2024
9 checks passed
@igamigo igamigo deleted the mFragaBA-add-creation-of-onchain-notes branch April 9, 2024 21:18
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.

3 participants