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

Refactor DEX to fit tx limits & misc improvements #863

Merged
merged 29 commits into from
Apr 25, 2023

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Apr 21, 2023

fixes #847
fixes #848

Various snarkyjs improvements that helped me achieve this DEX refactor:

  • fix the way that transaction limits are counted (the protocol counts the fee payer as signed account update, while snarkyjs didn't count it before)
  • add state.getAndAssertEquals() and state.fromAppState(), doccomment all state methods
  • make token APIs (mint, burn, transfer) accept account updates or smart contracts as sender/receiver (to avoid creating unnecessary account updates)
  • add an advanced option to reduce to skip adding the action state precondition (necessary for when we want to reduce actions on a different account than our own and need permissions for doing so)
  • add reducer.forEach(actions, ...) which is just reduce(actions, ...) without a state
  • Make the { fromActionState, .. } config optional in all versions of getActions
  • Make much heavier use of AccountUpdate.label in many methods that create account udpates, to make tx.toPretty() output easier to understand. Also, add a label for the fee payer.
  • Fix to the @method wrapper: use cloned method inputs for hashing the function signature for callData, so that the hash is not changed by the method mutating the inputs
  • Fix to the @method wrapper: in the composability case, no longer assume that MayUseToken.InheritFromParent is the correct default for the caller method (this was based on an earlier version of mayUseToken plus a misunderstanding about how it was supposed to work)
  • Export TokenId directly instead of a never-used Token class which contained the TokenId module as (hard to find) static Token.Id
  • add Permissions.allImpossible() for a set of permissions where nothing is allowed (more convenient than Permissions.default() when you want to make most actions impossible)

@mitschabaude mitschabaude changed the title Refactor DEX to fit tx limits & misc SnarkyJS improvements Refactor DEX to fit tx limits & misc improvements Apr 21, 2023
@mitschabaude mitschabaude marked this pull request as ready for review April 24, 2023 15:28
@mitschabaude
Copy link
Member Author

TODO: still have to update the changelog

Copy link
Contributor

@MartinMinkov MartinMinkov left a comment

Choose a reason for hiding this comment

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

Additional Question: Would it be worth hooking this up to CI and the Voter app? It doesn't have to be in this PR or anything cc @jasongitmail @nicc

LGTM! 🥳

src/examples/zkapps/dex/dex.ts Show resolved Hide resolved
@@ -21,11 +21,57 @@ export { assertStatePrecondition, cleanStatePrecondition };
* Gettable and settable state that can be checked for equality.
*/
type State<A> = {
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling for the help of @barriebyron here for these doc comments 😄 Would you happen to have any suggestions for the wording of these doc comments? Just for context, these comments will be shown to developers when they inspect the methods and in our API Reference docs.

Copy link
Contributor

@barriebyron barriebyron left a comment

Choose a reason for hiding this comment

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

I suggested some text changes, lmk if helpful @MartinMinkov and teach me where feedback is ineffective, ty!

src/examples/zkapps/dex/dex-with-actions.ts Outdated Show resolved Hide resolved
src/examples/zkapps/dex/dex-with-actions.ts Outdated Show resolved Hide resolved
src/examples/zkapps/dex/dex-with-actions.ts Outdated Show resolved Hide resolved
src/examples/zkapps/dex/dex-with-actions.ts Outdated Show resolved Hide resolved
src/examples/zkapps/dex/dex-with-actions.ts Outdated Show resolved Hide resolved
src/examples/zkapps/dex/dex-with-actions.ts Show resolved Hide resolved
src/examples/zkapps/dex/dex-with-actions.ts Show resolved Hide resolved
src/lib/state.ts Outdated Show resolved Hide resolved
mitschabaude and others added 3 commits April 25, 2023 08:57
Co-authored-by: Barrie Byron <barrie@o1labs.org>
Co-authored-by: Barrie Byron <barrie@o1labs.org>
fixes issue when not analyzing methods in dex example
@mitschabaude mitschabaude merged commit 7342fe5 into main Apr 25, 2023
9 checks passed
@mitschabaude mitschabaude deleted the testing/refactor-dex branch April 25, 2023 16:10
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.

Deploy DEX sample zkApp to Berkeley Update DEX zkApp to use events and actions
4 participants