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 ternary big integers #107

Merged
merged 7 commits into from
Jul 21, 2020

Conversation

thibault-martinez
Copy link
Member

Description of change

Add ternary big integers.

Links to any relevant issues

#77

How the change has been tested

Describe the tests that you ran to verify your changes.

Make sure to provide instructions for the maintainer as well as any relevant configurations.

Change checklist

Tick the boxes that are relevant to your changes, and delete any items that are not.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing unit tests pass locally with my changes
  • I have updated the CHANGELOG.md, if my changes are significant enough

bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/i384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/u384/mod.rs Outdated Show resolved Hide resolved
bee-crypto/src/ternary/bigint/u384/mod.rs Show resolved Hide resolved
bee-crypto/src/ternary/bigint/u384/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@samuel-rufi samuel-rufi left a comment

Choose a reason for hiding this comment

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

It's a quite complex RFC, however from the perspective of the code - besides what Joshua and Alex noticed - it seems ok to me.

Alex6323
Alex6323 previously approved these changes Jul 20, 2020
Copy link
Contributor

@Alex6323 Alex6323 left a comment

Choose a reason for hiding this comment

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

It is one of the hardest to understand Bee crates so far, and I would need to really deep-dive into the details of it to fully grasp it. I have no doubt, that the code is correctly working as it's cross-checked with other implementations via unit-tests, but the code overall feels a bit over-engineered, and also not very DRY, although I am not sure, the repetitiveness could have been prevented here.

@samuel-rufi samuel-rufi self-requested a review July 20, 2020 16:43
@thibault-martinez thibault-martinez merged commit ce71e27 into iotaledger:master Jul 21, 2020
@thibault-martinez thibault-martinez deleted the crypto-bigint branch July 21, 2020 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-feature Category - Feature e-high Experience - High p-critical Priority - Critical wg-crypto Working Group - Crypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants