Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

chore!: refactor ToRadix to ToRadixLe and ToRadixBe #58

Merged
merged 20 commits into from
Feb 16, 2023

Conversation

Ethan-000
Copy link
Contributor

@Ethan-000 Ethan-000 commented Jan 19, 2023

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves #57

Description

refactor ToRadix to ToRadixLe and ToRadixBe

Summary of changes

The only key change is here https://github.com/Ethan-000/acvm/blob/2d1fdf892114e571a39713be172977dfadbeca22/acvm/src/pwg/directives.rs#L99

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

add test to ToRadixRe similar to the others

(If applicable.)

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

(If applicable.)

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! Left one minor style comment to try out

acvm/src/pwg/directives.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

I actually did not mean to approve yet I hope this dismisses

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

I am thinking it would be nicer to actually not have two separate enums for ToRadix.

Let's switch to something like this:

        Directive::ToRadix { a, b, radix, is_little_endian } => {
            let val_a = get_value(a, initial_witness)?;

            let a_big = BigUint::from_bytes_be(&val_a.to_be_bytes());
            let a_dec = if is_little_endian {
               a_big.to_radix_le(*radix)
            } else {
               a_big.to_radix_be(*radix)
            }
            match to_radix_outcome(b, &a_dec, initial_witness) {
                Ok(()) => Ok(()),
                Err(e) => Err(e),
            }
        }

@kevaundray kevaundray changed the title refactor ToRadix to ToRadixLe and ToRadixBe chore!: refactor ToRadix to ToRadixLe and ToRadixBe Feb 3, 2023
acvm/src/pwg/directives.rs Outdated Show resolved Hide resolved
acvm/src/pwg/directives.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

One nit pick comment. Once the minor merge conflicts are fixed this looks good to me :)

acvm/src/pwg/directives.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor

I have a few nits, but I appreciate how long this PR has been up so will address these in a separate PR. Btw Thank you for fixing this issue Ethan, your efforts are really appreciated :)

@Ethan-000
Copy link
Contributor Author

Thank you guys for reviewing it 🙂!! Suggestions on details are very much welcome cus sometimes I wasn't paying attention and it helps me write better code. I didn't really notice for loop and if don't need ; for example

@vezenovm vezenovm merged commit 2427a27 into noir-lang:master Feb 16, 2023
@github-actions github-actions bot mentioned this pull request Feb 16, 2023
kobyhallx pushed a commit that referenced this pull request Jul 26, 2023
* chore: bump bberg commit to match `noir-lang/noir`

* fix: support usage of pedersen with non-zero `domain_separator`

* chore: fix minimum amount of memory for bberg wasm
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add both little endian and big endian form of to radix
3 participants