- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8
          Transition to Integer trait
          #36
        
          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
Conversation
| 
 | 
| Almost, yes, save for  | 
| @fjarri aside from a link to the Montgomery representation, this should take care of the other issues: RustCrypto/crypto-bigint#367 | 
| It takes care of some of them, but not all - e.g. vartime ops, bit operations, division, etc are still not in traits. Also it would be nice to have  | 
| Feel free to open PRs to add whatever functionality you'd like which has common impls on both types. Or just tell me specifically what you want and I can get them added. The bit operations should be fine to add (although  Division is present but perhaps you want something more. The  I don't think there's really a way to bound on impls on the reference type in a trait's bounds itself. You can do that with extra  | 
| I'd like to help out with the implementation of core functionalities using  It seems like there are a lot of functions parameterized by  Thank you! | 
| This PR already includes all the necessary transition to numeric traits instead of  | 
| 
 After pulling this branch it seems that the trait in  Thank you! | 
| @tarcieri has been implementing necessary methods for  | 
        
          
                Cargo.toml
              
                Outdated
          
        
      | [dependencies] | ||
| crypto-bigint = { version = "0.6.0-pre.0", default-features = false, features = ["rand_core"] } | ||
| #crypto-bigint = { version = "0.6.0-pre.0", default-features = false, features = ["rand_core"] } | ||
| crypto-bigint = { git = "https://github.com/RustCrypto/crypto-bigint.git", default-features = false, features = ["rand_core"], commit = "5ee582b2d1b7077f1e0737210506c53193c24939" } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: if you check in Cargo.lock it will pin this for you
| @xuganyu96 the relevant trait in  | 
| 
 @tarcieri @fjarri I've taken a try and found the scope of work manageable (see #37 for my progress). If you don't mind I can take over the remainder of the transition (I am a grad student currently on winter break and so I'm looking for work to do anyways). Thank you! | 
| Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   99.21%   99.20%   -0.01%     
==========================================
  Files           9        9              
  Lines        1402     1386      -16     
==========================================
- Hits         1391     1375      -16     
  Misses         11       11              ☔ View full report in Codecov by Sentry. | 
| Ok, finally the tests pass. Remaining things in  
 @tarcieri, what's your take on this? | 
| The reason  Traits for vartime bitshifts sound OK. There are already various bit counting methods defined on  
 Something like how  How would  | 
| 
 
 
 Well, that's the question: in case of  | 
| On a side note, I think  | 
| 
 If the number fits in one  | 
| 
 We'd need a fallible trait/method to cover both cases. 
 It matches  | 
| 
 Not really.  In fact,  | 
| 
 
 
 In my attempt here,  fn main() {
    // with BoxedUint, separating bit_length and bits_precision is entirely understandable
    // "I want a random number that takes 128 bits to store and 64 bits to represent
    let random_odd = BoxedUint::random(&mut OsRng, 64, 256);
    // with Uint, bits_precision is exactly Uint::BITS, which might be a bit confusing
    // but with clear documentation and some `assert!` I think we can communicate the idea to the users
    let random_odd: U256 = U256::random(&mut OsRng, 64, U256::BITS);
}In practice this means that functions like  let stacked_prime: U256 = generate_prime(&mut OsRng, 64, U256::BITS);
let heaped_prime: BoxedUint = generate_prime(&mut OsRng, 64, 256); | 
3a45eda    to
    08e9813      
    Compare
  
            
          
                src/uint_traits.rs
              
                Outdated
          
        
      | impl UintLike for BoxedUint { | ||
| fn set_bit_vartime(&mut self, index: u32, value: bool) { | ||
| if value { | ||
| *self |= Self::one() << index | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BoxedUint::one doesn't actually have the same size as self. This should probably be replaced with Self::one_with_precision(self.bits_precision())
7fe8881    to
    e9c3319      
    Compare
  
    Integer trait
      | The two uncovered lines in  | 
Potentially fixes #34
In order to add the support for
BoxedUintwe need to make all the methods parametrized by someUintLiketrait instead of the number of limbs inUint. This PR attempts to see what kind of methods are necessary. Seeuint_traits.rsfor what needs to be implemented incrypto-bigint.UintLikeandUintModLikecan potentially be split into smaller traits.Notes:
::BITSanymore, so we need to change the API to getbit_length: usizeinstead ofOption<usize>everywhere&Self, which means there are some potentially unnecessaryclone()s involved, especially inlucas()(unless we want to write down scary trait bounds, which we really don't).