-
Notifications
You must be signed in to change notification settings - Fork 116
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
Implemented avoid throwing an error if private key is missing unless .send() is executed #931
Conversation
…l wallet sign the account update
Unfortunately I am having huge issues building, should it be the following? git submodule update --init --recursive
npm install
npm run build:node I could not find the info in current https://github.com/o1-labs/snarkyjs/blob/e55c71d1c47011c78d8c23a0db0eae394ff2c8af/CONTRIBUTING.md guide. Another problem I face is npm run test gives
my bash version is |
Even if tests are passing and review is positive please hold on with merging until I test with AURO. |
I also plan to PR https://github.com/o1-labs/snarkyjs-bindings/blob/main/mina-transaction/gen/transaction.ts#L112 change this to
|
thanks a lot for your efforts @marekyggdrasil! Yes, as you found out you need
that file is auto-generated, so please don't edit it. We also need a signature on the fee payer for some interactions with OCaml. Right now in snarkyjs we use a dummy signature if we need to |
…required for OCaml code
Thank you @mitschabaude for your comments! It's very helpful. I updated the code to use dummy signature to indicate signature is missing. This should not violate the OCaml code. If you have any other suggestions please feel free to share. In the meantime I will test it with AURO and see if I can fund new account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks @marekyggdrasil! We should also make the same behavior change for non-fee payer signatures, so that this fixes the problem in all cases. But I won't block the PR on that
Don't worry @mitschabaude I'm happy to handle that. Here's some update. I tested with AURO and SnarkyJS no longer blocks the transaction building. AURO seems to successfully sign it and post it. Below you can find a zkApp deployment transaction that was signed and broadcasted using AURO wallet. https://berkeley.minaexplorer.com/transaction/5JvA3mhiWmQxiak2dsh3vRBT355tGBxy6mYbpFeJ19FHqJPCzDEr Unfortunately, the transaction was failed anyway, due to another reason As always, many thanks! |
@marekyggdrasil Yeah you used the same nonce on a normal account update (for the fee payer account) as for the fee payer. SnarkyJS should handle that correctly if you tell it what the fee payer is, with |
src/lib/account_update.ts
Outdated
// there is a change signature will be added by the wallet | ||
// if not, error will be thrown by verifyAccountUpdate | ||
// while .send() execution | ||
return accountUpdate as AccountUpdate & { lazyAuthorization?: LazyProof }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're lying about the type here, I'm not sure if that's a good idea :/ Could you instead at least set the signature to an empty string or dummy signature, and use Authorization.setSignature
? that way, at least the types are correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update, thanks for noticing this! I just copy-pasted badly...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, looks good to merge now!
@mitschabaude you were right! Behold the first in history zkApp deployed via UI using AURO wallet! https://berkeley.minaexplorer.com/transaction/5JvRpM56zg5goMez3Kw9sj5vLakn6B5xRZeu4pgc2VnucYPtpEyX Got any champagne? ;) Thanks for pointing out the type signature problem. Of course I'll fix and I'll also add a test to check if lack of private key is detected while |
🍾 |
@mitschabaude would you approve of adding an integration test of transaction building? Thinking of what would be a good way to test it. From what I see none of the AccountUpdate validators are tested currently. |
Yeah I would approve. FWIW we have lots of tests that test transaction building from various perspectives, for example all the "integration tests" that CI runs have some form of local zkApp e2e flow. But I'm very much in favor of adding tests for particular aspects, like signing here. You could make it a |
@mitschabaude I implemented a jest test case, thanks for the hints! I have just one more suggestion. I noticed that the bindings is perfectly capable to detect the missing signature. Before even my AccountUpdate validation is executed, the bindings already throws |
That's interesting! According to my reading of the code, that's unexpected. But I think even if the check is redundant, it doesn't hurt to leave it in. I'll investigate this when there's time, but the code at this point seems solid enough and I'd be in favor of merging |
Thanks @mitschabaude for your time! If anything else is needed feel free to ping me, happy to help with Snarky! |
This is a WIP PR for resolving the issue #930
Issue discovered while working on https://github.com/sqrt-xx/mina-ui-deployment
The related discussion thread is: https://discord.com/channels/484437221055922177/1109079490111479848