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

Add args to the callback method invocation #516

Merged
merged 2 commits into from
Oct 26, 2022

Conversation

MartinMinkov
Copy link
Contributor

Description

Fixes #515
This PR adds a spread operation of the arguments into the called method on a callback. This properly passes all argument values into the method invocation.

Tested:

Manual testing

Copy link
Member

@Trivo25 Trivo25 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

Good catch!

@MartinMinkov MartinMinkov merged commit 928bc47 into main Oct 26, 2022
@MartinMinkov MartinMinkov deleted the fix/missing-arguments-callback branch October 26, 2022 23:43
MartinMinkov added a commit that referenced this pull request Nov 15, 2022
* changelog

* proper link

* make ./run handle files with internal imports

* make AccountUpdate a valid method argument

* add `this.authorize` to adopt update & witness its children

* restore old ./run behaviour if the new one fails

* fix token example

* use struct

* remove redundant function

* tweak run script again

* wip  callback refactor

* fixup

* update bindings

* Fetch Account's verification key.

* make tx.send async

* add error trace

* helpers for pretty-printing updates

* update changelog

* make tx.send async in all examples and tests

* More details on what to check when a state fetch fails

* fix token test

* tweak witness to just get & return fields

* clone updates when witnessing, so prover doesn't interfere with tx
-> to still be able to identify the same update, give them an id

* fixes

* avert future name clash

* make unit tests rely on existing build
(so that Mina CI can be independent of checked-in bindings)

* merge accountUpdateFromCallback into authorize

* changelog

* make child layout spec more verbose and understandable

* update bindings

* fix returning values in the top-level transactoin case

* reenable having no authorization when calling zkapps

* Dex Application - supplyLiquidity Happy Path

* Use predefined keys for debugging purposes

* Add getBalance function to TokenContract

* Refactor supplyLiquidity methods to remove return type and use getBalance

* Update dex run script to use supplyLiquidity methods

* add enableProofs

* add isDelegateCall and  compute `caller` from it

* fix logic to tompute caller in the prover

* set delegate call in authorize, clone

* wip get dex running

* wip add isDelegateCall to circuit

* caller progress

* label acount updates

* no delegate call from the top level

* make token send go through

* Add failure cases for supply liquidity in DEX

* Add proof verification into the tests.

* Add proof verification into the tests.

* Fix for tests (Proof verification).

* Skipping some tests in CI condition refactoring.

* Commenting out txn signing to allow proving..

* add error for long token symbol & make it a struct

* fix dependency cycle

* make isDelegateCall a Bool

* remove caller hacks from dex/run

* remove unnecessary code

* remove debugging

* fix jest tests

* make SequenceEvents exportable for external use

* delay children hashing until caller is known, fix logic to know when we're at the top level

* update bindings

* instantiate subclass of struct, not base class

* Remove clone for lazyAuthorization

* Update dex example

* better struct fix

* fix dummy proof logic

* fixes to dex contract logic & balance sum

* minor

* handle more cases when making account update a child

* try setting some zkapps to delegate_call, but creates different token owner issues

* typos

* make redeem liquidity work

* minor

* Simplify things by utilizing test fixtures.

* enable changing proofsEnabled

* dex debugging with proofs

* make Circuit.witness create a new unchecked snark context, and only call `exists` in checked mode

* fix assertion about hashed child

* any children

* fix demo

* make tx sending async

* temp fix

* fix integration test

* add verification key test

* minor

* allow children which can't delegate token spending power, as default for authorize

* wip swap happy path

* missing access permission issue repro

* fix dummy proof for child updates

* fix remaining bugs

* fix tests

* remove comment

* swap fix

* refine the logic for when `exists` is called

* Circuit.log

* stop implicitly attaching account updates to tx

* remove debugging

* remove unused z token

* changelog

* remove redundant checks guarding Circuit.witness

* fix precondition tests

* changelog

* fix attach logic

* add edgecase with proof verification

* rm unused code

* fix broken permission and vk tests

* manually specify feepayer nonce

* dont fetch nonce if nonce is specified

* address feedback

* add Uint.toBigInt

* helper to check token balances

* add checks for supply liquidity (happy path)

* checks for redeem liquidity (no vesting)

* fix nonce calculation

* start adding timing, but missing error on repeated liquidity supplying

* pretty printing improvements

* expose account timing

* use ES private class fields instead of `private`

* roll back one private field bc its not inherited

* better interface for setting local global slot

* add vesting

* finish redeem liquidity tests

* swap add checks for happy path

* working on overflow tests

* Add optional assertion failure messages to SnarkyJS methods (#470)

* Add optional error message to Field.assertEquals

* Add optional message for Bool.assertEquals

* Add optional message for Group.assertEquals

* Add optional message to Field.assert compares

* Add optional message to Field.assertBoolean

* Add optional message to Bool.assertTrue/False

* Changelog

* feat: Add optional message for UInt64 assert methods

* feat: Add optional message for UInt32 assert methods

* feat: Add optional message for Int64 assert methods

* Update bindings

* merge in existing supply failures and add the rest

* bonus test for the supplying after lock period

* run tests with and w/o vesting

* fix uint comparisons

* push fix to examples

* add unit test for division in the prover

* fix second run by recreating ledger

* update bindings

* add back setGlobalSlot

* add back setGlobalSlot

* Add args to the callback method invocation (#516)

* Add args to the callback method invocation

* Changelog

* remove static from methods

* remove static definitions for field

* update changelog

* fix internal usage

* start snarky doc comments

* Removes static one/zero/-1 from Field

Part of #511

* Deprecates instead of removes Field members

* Add shutdown to end of primitive unit test (#525)

* Add timeout minutes to github action (#526)

* dump initial mina-signer experiments

* type check mina-signer

* make transaction-helpers generic over base types

* remove most specializations to base types away

* make transaction-helpers fully generic

* lift customTypes to functor level

* rename

* change dir and filenames

* remove undefined from typemap

* remove primitive types from typemap

* autogenerate type map

* move everything to common folder

* update bindings

* fix tests

* Update int.test.ts

* fix tests

* redo merge conflict fix

* Add additional token tests (#522)

* Refactor token tests and add additional tests

* Add comment descriptions

* bump node CI time

* Rename `authorize` to `approve` (#534)

* Rename 'authorize' to 'approve'

We rename 'authorize' to approve in the aims to take advantage of the
concept of 'approve' in Ethereum. We hope that developers will
intuitively understand what it means to 'approve' a AccountUpdate given
that a smart contract in Ethereum must get approval to spend tokens.

* Changelog

* Update tokens test

* Make `JSONValue` internally used only (#536)

* Make JSONValue internally used only

Move the definition of JSONValue into circuit_value.ts since there are
many types that depend on JSONValue inside that file. We also removed
any usages of JSONValue from public fromJSON methods since that type is
intended for internal use.

* Replace JSONValue with any type

* Fix tests

Update int.test.ts

fix tests

redo merge conflict fix

 Fix tests

* Removed all uses of JSONValue with any

* cache nonce

* minor

* make fetch tests run with CI && import from fetch

* make tests abort

* more tests

* add nested escaped string

* add init and show opt-in proving for it in example

* fix init logic so that we can assure its only run once

* tweak error

* fix token test

* better type for numberOfRuns

* swap import order

* adjust tests

* fix dex

* changelog

* fix error parsing logic

* replace links

* enable manually triggering a workflow

* fix yaml

* move Reducer and MerkleStuff off Experimental

* update examples

* move token and approve off this.experimental

* update examples

* changelog

* fix merkle tree tests

* fix token test

* deprecate this.sign and document requireSignature

* changelog

* changelog

* more comments

* more comments

* 0.7.0

* changelog

Co-authored-by: Serhii Shymkiv <sergey@shimkiv.com>
Co-authored-by: Florian <florian@technotro.com>
Co-authored-by: Brandon Kase <brandon.kase@gmail.com>
Co-authored-by: Martin Minkov <minkovlmartin@gmail.com>
Co-authored-by: Comdex <wcomdex@foxmail.com>
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.

Callback arguments list is always undefined
3 participants