Depends on D631
Details
Details
- Reviewers
schancel deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGINGa9e694c25063: Support for encoding addresses using cashaddr.
rABCa9e694c25063: Support for encoding addresses using cashaddr.
Added unittests.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- 10-23-cashaddr
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1039 Build 1039: arc lint + arc unit
Event Timeline
| src/cashaddrenc.cpp | ||
|---|---|---|
| 18 ↗ | (On Diff #1603) | Does this basically operate as a closure then? |
| 52 ↗ | (On Diff #1603) | What's the point of constructing this in such a way if we have a single-use function that takes params? |
| 59 ↗ | (On Diff #1603) | This was added in another patch already? |
| src/cashaddrenc.h | ||
| 15 ↗ | (On Diff #1603) | I assume this needs chainparams to add the appropriate prefix and whatnot? |
| src/test/cashaddrenc_tests.cpp | ||
| 25 ↗ | (On Diff #1603) | Maybe we should generate a bunch of these? |
Comment Actions
A few nits, but I think the double padding problem can be an issue. This needs to be accounted for and tested. Except that, it's good.
| src/cashaddrenc.cpp | ||
|---|---|---|
| 40 ↗ | (On Diff #1603) | We need to make sure we don't double pad when encoding/decoding. This needs more logic and a test for it. |
| 69 ↗ | (On Diff #1603) | Please use uint8_t |
| 80 ↗ | (On Diff #1603) | I think it'd be preferable to use std::copy but not a big deal. |
Comment Actions
- Convert entire data-part with ConvertBits. Used to convert the hash only.
- Ignore extra padding
- Added test to encode/decode bunch of random addresses.
- Explicitly check that we get the expected size when converting bits 8 <-> 5 and back.
- Use std::copy instead of memcpy
| src/cashaddrenc.cpp | ||
|---|---|---|
| 52 ↗ | (On Diff #1603) | This is how boost suggests working with boost::variants http://www.boost.org/doc/libs/1_64_0/doc/html/variant.html#variant.motivation |
| src/cashaddrenc.h | ||
| 15 ↗ | (On Diff #1603) | Yes, it needs the address prefix. |
Comment Actions
There should be a tests using actual addresses in there. If nothing else, they can be used as test vectors.
| src/test/cashaddrenc_tests.cpp | ||
|---|---|---|
| 92 ↗ | (On Diff #1636) | braces |
| src/test/cashaddrenc_tests.cpp | ||
|---|---|---|
| 162 ↗ | (On Diff #1655) | Yes no, that was stupid. |