Issue #89 - Initial work on adding checksum option to get_new_addresses #106
Conversation
…to get_new_addresses
|
Hey @plenarius thanks for your pull request!! I will take a look this weekend! |
todofixthis
left a comment
todofixthis
left a comment
There was a problem hiding this comment.
This looks really good, thank you!!
I requested a few changes, but it's all minor stuff. Looking forward to merging this!
examples/address_generator.py
Outdated
| def main(uri, index, count): | ||
| # type: (Text, int, Optional[int], bool) -> None | ||
| def main(uri, index, count, security, checksum): | ||
| # type: (Text, int, Optional[int], Optional[int], Optional[bool]) -> None |
There was a problem hiding this comment.
Optional[bool] means that the value can be either a boolean or None (PEP-484).
In this case, though, None is not an expected value for checksum, so we don't need the Optional part.
| type = int, | ||
| default = 2, | ||
| help = 'Security level to be used for the private key / address. Can be 1, 2 or 3', | ||
| ) |
There was a problem hiding this comment.
This is a great idea! Good thinking!
Minor thing: Could we replace the 2 with AddressGenerator.DEFAULT_SECURITY_LEVEL?
| default = False, | ||
| dest = 'checksum', | ||
| help = 'List the address with the checksum.', | ||
| ) |
iota/api.py
Outdated
| checksum = False, | ||
| ): | ||
| # type: (int, Optional[int], int) -> dict | ||
| # type: (int, Optional[int], int, Optional[bool]) -> dict |
There was a problem hiding this comment.
Optional not needed, etc., etc.
| :param checksum: | ||
| Specify whether to return the address with the checksum. | ||
| Defaults to False. |
There was a problem hiding this comment.
I feel like there's some documentation we could include a link to here, but I can't find any ¯\_(ツ)_/¯
Side note: typing the shrug dude in a Markdown editor sure is tricky!
| | f.Max(self.MAX_SECURITY_LEVEL) | ||
| | f.Optional(default=AddressGenerator.DEFAULT_SECURITY_LEVEL), | ||
|
|
||
| 'checksum': f.Type(bool) | f.Optional(default=False), |
There was a problem hiding this comment.
👍
f.Optional specifies a default value that will be used if the incoming value is None:
https://filters.readthedocs.io/en/latest/filters_list.html#miscellaneous-filters
This filter is occasionally important, because all the other filters will treat None as a passthrough value by default:
https://filters.readthedocs.io/en/latest/getting_started.html#much-ado-about-none
| 'count', | ||
| 'index', | ||
| 'securityLevel', | ||
| 'checksum', |
There was a problem hiding this comment.
Could we keep these strings in alphabetical order, pretty please?
| if self.checksum: | ||
| return self.address_from_digest(self._get_digest(key_iterator)).with_valid_checksum() | ||
| else: | ||
| return self.address_from_digest(self._get_digest(key_iterator)) |
There was a problem hiding this comment.
All that work just for 3 extra lines of code :P
| self.assertDictEqual( | ||
| response, | ||
| {'addresses': [self.addy_1_checksum]}, | ||
| ) |
There was a problem hiding this comment.
These tests look great, thank you!
Since AddressGenerator._generate_address() now also has a checksum argument, could we add a test to AddressGeneratorTestCase as well?
That way, if we ever modify GetNewAddressesCommand and change the way it does checksums, we will still have coverage for AddressGenerator's checksum functionality.
| """ | ||
| # type: (Seed, int, Optional[int]) -> List[Address] | ||
| generator = AddressGenerator(seed, security_level) | ||
| # type: (Seed, int, Optional[bool]) -> List[Address] |
There was a problem hiding this comment.
I think this guy's a typo, actually. The type hint goes above the docstring, not under it (PEP-484). You can remove this line entirely.
… AddressGenerator.DEFAULT_SECURITY_LEVEL instead of hardcoded 2 in example, use addy.address for checksum-less address
todofixthis
left a comment
todofixthis
left a comment
There was a problem hiding this comment.
This looks great! Thank you @plenarius!
…checksum Issue iotaledger-archive#89 - Initial work on adding checksum option to get_new_addresses
2 participants
Okay first shot at contributing. Still getting my feet wet with the API but this looked like something to get started. My attempt at Issue #89
Should I have added the --security flag in the example address_generator.py in another commit?
The FindTransactions part with me having to chop the checksum back off, feels like that's pretty hackish. And I wasn't entirely sure where the best place to put the with_valid_checksum() so I settled on in the _generate_address().
Anyway it's a start! #