Entropy, Mnemonic, Seed as classes with validation on instantiation

mvondracek commented 5 years ago

Objects would also provide us with EAFP, imutability, and e. g. separation of mnemonic and mnemonic checksum, etc.

Related #11, #12, #22, #24.

mvondracek commented 5 years ago

Would it be like taint? Input as bytes/str would be considered tainted, while Entropy, Mnemonic, and Seed would be withnout taint as it passed validation on instantiation. 👍

sobuch commented 5 years ago

If we are doing this, then probably whole function mnemonic2entropy should be used as init for mnemonic, so that we can store the enrtopy and dont have to do the same thing twice

sobuch commented 5 years ago

also, it might be better to make another class out of all the functions and move get_dictionary to its init

mvondracek commented 5 years ago

also, it might be better to make another class out of all the functions and move get_dictionary to its init

But dictionary is still read only once.

mvondracek commented 5 years ago

Let's finish some working version of package and tool first. We still need to focus on analytic tools.

mvondracek commented 5 years ago

sobuch commented 5 years ago

to make the process easier, can we merge remaining PR's? I would propose to also merge current dev to master before that

mvondracek commented 5 years ago

Sure, but #19 requires mnemonic validation at least for verify.

sobuch commented 5 years ago

I mean, I could branch out from https://github.com/mvondracek/PA193_mnemonic_Slytherin/tree/task-11-entropy-mnemonic-transformations branch, but we are just adding more work with resolving conflicts for future in case dev will move until I am done.

mvondracek commented 5 years ago

Currently, branch 11-entropy-mnemonic-transformations contains bug introduced in that branch. I think that the bug should be fixed and branch merged to dev before doing more changes.

sobuch commented 5 years ago

Classes implemented in https://github.com/mvondracek/PA193_mnemonic_Slytherin/tree/task-26-class-representation , however tests need to be fixed - some were always incorrect, most just need to use the classes

sobuch commented 5 years ago

As is, it looks kind of messy to me, I would like it more if we could use functions like entropy.toMnemonic, mnemonic.generate_seed and so on, however work with dictionary complicates this. I believe https://github.com/mvondracek/PA193_mnemonic_Slytherin/issues/26#issuecomment-542393717 would help here, @mvondracek @lsolodkova do you see better solution or can I turn our solution into one class with just 3 member classes, 3 public functions and init?

mvondracek commented 5 years ago

Hi, thanks for working on it. However, I don't see reason why you deleted functions like is_valid_entropy. As was discussed on Hangouts, we could use just simple classes with constuctors and factory methods which would call our already defined functions for validation and conversion.

As you previously pointed out, it would be better if we finish current PRs first, before doing this refactoring. Could you please fix #19 first?

sobuch commented 5 years ago

well https://github.com/mvondracek/PA193_mnemonic_Slytherin/issues/28 is already fixed here and this needs to be merged into https://github.com/mvondracek/PA193_mnemonic_Slytherin/pull/19 anyway, so when the tests are fixed here, https://github.com/mvondracek/PA193_mnemonic_Slytherin/pull/19 will be fixed. But if we would eventually switch to one class, we would need to fix the tests again, so i think now is the best time to decide whether we want to do it or not.

I moved functionality from is_valid_entropy to Entropy.init Sorry, but whats the point of separate function which would be called only from that constructor anyway? Even for testing, we dont want to test that we can detect (in)correct entropy, but rather that the instantiation of Entropy class works correctly, which means testing Entropy(xyz).

mvondracek commented 5 years ago

I moved functionality from is_valid_entropy to Entropy.init Sorry, but whats the point of separate function which would be called only from that constructor anyway? Even for testing, we dont want to test that we can detect (in)correct entropy, but rather that the instantiation of Entropy class works correctly, which means testing Entropy(xyz).

Sorry, you're right with that init+validation. I was thinking about 'factory methods which would call our already defined functions for conversion'. That's fine.

I've just created draft PR for this and will give there few comment. Let's merge it as soon as the tests pass. :)

lsolodkova commented 5 years ago

about 'factory methods which would call

Occam's razor is any gentleman's best friend.

mvondracek / PA193_mnemonic_Slytherin

Entropy, Mnemonic, Seed as classes with validation on instantiation #26