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

fix: Add checks for arg-type #638

Merged
merged 4 commits into from
May 22, 2023
Merged

fix: Add checks for arg-type #638

merged 4 commits into from
May 22, 2023

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented May 22, 2023

Description

Fixed #611

Extends encodeArgument() for the field case to support arg of number, bigint, Field or types that implement a toField() function.

Note that it will insert arg directly if arg != bigint && !arg.toField().

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@LHerskind LHerskind requested a review from Maddiaa0 May 22, 2023 12:13
Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

I approve but i also commited so looping in @rahul-kothari

@Maddiaa0 Maddiaa0 requested a review from rahul-kothari May 22, 2023 14:32
@rahul-kothari rahul-kothari force-pushed the lh/611-encode-arguments branch from f372084 to 6921cb9 Compare May 22, 2023 14:51
@rahul-kothari
Copy link
Contributor

@LHerskind there was an E2E test I found where we were still encoding even though it is not required anymore. Idk if there are anymore places though.

(I simply searched for all instances where we call methods on a contract i.e. by searching for .methods() in the codebase)

@LHerskind LHerskind merged commit 4ffbe9c into master May 22, 2023
@LHerskind LHerskind deleted the lh/611-encode-arguments branch May 22, 2023 15:07
jeanmon pushed a commit that referenced this pull request May 23, 2023
* fix: Add checks for arg-type

* fix: support arg of 'number' type

* fix: correct wrapped fr in acir sim

* no need to force encode in some e2e tests!

---------

Co-authored-by: Maddiaa0 <[email protected]>
Co-authored-by: Rahul Kothari <[email protected]>
suyash67 pushed a commit that referenced this pull request May 25, 2023
* prelim / very-broken commit where Jean and I started implementing the initial private kernel and moving logic between it and the inner kernel

* wip - fix some compilation issues, remove unused imports, remove
update_end_values leftover

* wip - some minor changes and comments after pairing with David

* wip: additional compilation fixes

* Refactor helper functions in private kernel tests

* wip - Fix linking issue and comment out some tests

* wip - repair native and circuit tests

* wip - include using double quotes and remove unused code

* wip - fixing circuit_create_proof_cbinds test and adapting
private_kernel_sim function to the inner/init split

* wip - fix clang tidy issue

* wip - post master merger fixes and remove test on new nullifiers

* wip - fix clang tidy issue

* wip - fix clang tidy issue

* wip - fix typescript tests and data structure split

* wip - format typescript

* wip - replace CONSTRUCTOR_ARGS by FUNCTION_ARGS

* wip - add nullifier and l1-l2-message tree roots in initial private
kernel circuit

* wip - post merge fix

* Migrate common.hpp functions into non-templated ones

* refactor(sol): use Hash.sha256ToField library where required (#637)

* refactor(sol): use Hash.sha256ToField library where required

* fmt :)

* feat: expose bytes32 hash directly

* fmt: https://i.pinimg.com/originals/81/23/a1/8123a132c007eab782d6ca9bed517eb3.jpg

---------

Co-authored-by: Maddiaa0 <[email protected]>

* fix: Add checks for arg-type (#638)

* fix: Add checks for arg-type

* fix: support arg of 'number' type

* fix: correct wrapped fr in acir sim

* no need to force encode in some e2e tests!

---------

Co-authored-by: Maddiaa0 <[email protected]>
Co-authored-by: Rahul Kothari <[email protected]>

* fix: naming consistency in messaging (messageHash -> message) (#641)

* fix: contentHash -> content

* fix: update content in nr

* fix: update incorrect message hash naming

---------

Co-authored-by: Maddiaa0 <[email protected]>

* chore: remove unknown casts in archiver test (#648)

Removes intermediate casts to unknown from archiver test, which hide
type errors in the args of the mocked event.

* Consolidation code for contract_logic

* e2e fixing: Initialise public and private call stack and comment out some checks

* Renaming file native_private_kernel_initial.XPP into
native_private_kernel_init.XPP

* Rename native_private_kernel_circuit into
native_private_kernel_circuit_inner

* Simplify if/else clause on deployment contract boolean in contract logic

* abis test snapshot - restore from master for vk hash

* Remove old TODO comments

* Fork FUNCTION_ARGS into FUNCTION/CONSTRUCTOR_ARGS generator indices

* Move code copying public/private_call_stack in PKC init

* Clang tidy fix

* Add clarifications into a comment

* Added comments

* remove empty lines

---------

Co-authored-by: jeanmon <[email protected]>
Co-authored-by: Maddiaa <[email protected]>
Co-authored-by: Maddiaa0 <[email protected]>
Co-authored-by: Lasse Herskind <[email protected]>
Co-authored-by: Rahul Kothari <[email protected]>
Co-authored-by: Santiago Palladino <[email protected]>
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.

bug: Fields in encodeArguments gets wrapped in another field in acvm
3 participants