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

Replace toml_map_to_field and toml_remap with traits to map between InputValues and TomlTypes #677

Merged
merged 6 commits into from
Jan 31, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Jan 21, 2023

Related issue(s)

Description

Summary of changes

I've replaced the functions toml_map_to_field and toml_remap with implementations of TomlTypes::from<InputValue>() and InputValue::try_from<TomlTypes>(), i.e. rather than mapping the entire set of arguments at once we specify how to map a single input from one type to another.

We can then just iterate over each argument and map them individually.

Dependency additions / changes

N/A

Test additions / changes

N/A

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

jfecher
jfecher previously approved these changes Jan 23, 2023
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Looks great, just one style comment that you can choose to ignore if you wish. Seems like we need to re-run the CI as well.

crates/noirc_abi/src/input_parser/toml.rs Outdated Show resolved Hide resolved
@TomAFrench
Copy link
Member Author

Looks like this will end up incompatible with #622 due to it requiring context about whether a particular value is a field or a string which we can't pass to try_from. We could create some ad-hoc conversion method however where we can pass in the desired AbiType.

@vezenovm
Copy link
Contributor

Looks like this will end up incompatible with #622 due to it requiring context about whether a particular value is a field or a string which we can't pass to try_from. We could create some ad-hoc conversion method however where we can pass in the desired AbiType.

I like the move away from toml_map_to_field, however, as you mention this change will be incompatible with #622. I think we will have to add an ad-hoc method that accepts an AbiType in order for this to work with the changes for string inputs. That PR has some last changes that are being pushed through and should be closed soon

@jfecher
Copy link
Contributor

jfecher commented Jan 23, 2023

I believe the reason PR #622 adds the type-aware conversions is to keep allowing "0xabcd" in the toml to decode to an integer or field. Does toml not allow unquoted hexadecimal literals like 0xabcd or is there some maximum bits limitation in toml I'm not aware of? If toml allows the unquoted hex literals my vote would be to remove support for the quoted ones.

@vezenovm
Copy link
Contributor

I believe the reason PR #622 adds the type-aware conversions is to keep allowing "0xabcd" in the toml to decode to an integer or field. Does toml not allow unquoted hexadecimal literals like 0xabcd or is there some maximum bits limitation in toml I'm not aware of? If toml allows the unquoted hex literals my vote would be to remove support for the quoted ones.

To my knowledge toml does not allow unquoted hexadecimal literals and that is why we use strings for hexadecimal inputs. If toml does allow unquoted hex literals I agree we should just remove support for the quoted ones to avoid confusion.

@TomAFrench
Copy link
Member Author

Does toml not allow unquoted hexadecimal literals like 0xabcd or is there some maximum bits limitation in toml I'm not aware of? If toml allows the unquoted hex literals my vote would be to remove support for the quoted ones.

It does support hex representations of integers however integers are limited to values which fit in an i64 so without accepting string representations we wouldn't be able to specify a large range of fields.

@TomAFrench TomAFrench mentioned this pull request Jan 24, 2023
5 tasks
@TomAFrench TomAFrench force-pushed the input-toml-conversion-traits branch from 5f97e69 to 8aa0696 Compare January 30, 2023 08:13
@TomAFrench TomAFrench force-pushed the input-toml-conversion-traits branch from 8aa0696 to e23da12 Compare January 30, 2023 08:17
@TomAFrench TomAFrench requested a review from jfecher January 30, 2023 08:44
@TomAFrench
Copy link
Member Author

I've rebased this back on top of master to support string types.

jfecher
jfecher previously approved these changes Jan 30, 2023
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

vezenovm
vezenovm previously approved these changes Jan 30, 2023
@TomAFrench TomAFrench dismissed stale reviews from vezenovm and jfecher via c2e67d5 January 31, 2023 12:34
@TomAFrench TomAFrench merged commit d78739c into noir-lang:master Jan 31, 2023
@TomAFrench TomAFrench deleted the input-toml-conversion-traits branch January 31, 2023 14:44
TomAFrench added a commit that referenced this pull request Feb 3, 2023
* master:
  Rename methods that use `conditionalize` to be more descriptive (#739)
  feat(noir)!:  Returned values are no longer required by the prover (#731)
  chore: explicit versions for dependencies (#727)
  chore: readability improvements (#726)
  feat(nargo): include short git commit in cli version output (#721)
  Remove print to console for named proofs in `nargo prove` (#718)
  chore: clean up serde-related dependencies (#722)
  Handle out-of-bound errors in CSE (#471) (#673)
  Remove unused dependencies and only use workspace inheritance on shared deps (#671)
  feat(std_lib)!: modulus bits/bytes methods, and to_bits -> to_le_bits (#697)
  Implement numeric generics (#620)
  Review some TODO in SSA (#698)
  Replace `toml_map_to_field` and `toml_remap` with traits to map between `InputValue`s and `TomlTypes` (#677)
  Apply witness visibility on a parameter level rather than witness level (#712)
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.

3 participants