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

Authorization deserialization fix #2479

Draft
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

lukenewman
Copy link
Collaborator

Motivation

When deserializing an Authorization from string a call stack that included multiple transitions, the structural assertion logic would erroneously fail. This PR fixes the assertion logic but does not change the underlying structure/order of the Authorization fields.

Test Plan

Our (Puzzle) delegated proving stack takes in user-generated Authorizations in string form. We then parse these and execute the proof request using snarkVM crates. I've tested that these Authorization deserializations and subsequent proofs do indeed land onchain properly.

I can provide screenshots/videos on request. For now, you can see the following transaction that was generated through our stack and has landed onchain: https://testnet.aleoscan.io/transaction?id=at14d4edujs6ztghev2zppp55v4xlffu7q3p379dlvdsu3yuu2cluysd84ktk

I am also willing to create a sample project showing this fix in action.

A more formal test plan would involve embedding a multi-transition Authorization unit test into snarkVM itself and not just relying on a fee_public test case.

Related PRs

none

@vicsn
Copy link
Contributor

vicsn commented Jun 6, 2024

Great find!

embedding a multi-transition Authorization unit test

Yes please. You can see some recent examples of adjusted tests in ledger/src/tests.rs in: https://github.com/ProvableHQ/snarkVM/pull/13/files . There are also a lot in synthesizer/src/vm/mod.rs

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