Page MenuHomePhabricator

Support for encoding addresses using cashaddr.
ClosedPublic

Authored by dagurval on Oct 23 2017, 11:09.

Details

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dagurval created this revision.Oct 23 2017, 11:09
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 23 2017, 11:09

Split out of D544

schancel accepted this revision as: schancel.Oct 26 2017, 16:13
schancel added a subscriber: schancel.
schancel added inline comments.
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?

deadalnix requested changes to this revision.Oct 26 2017, 16:58
deadalnix added a subscriber: deadalnix.

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.

This revision now requires changes to proceed.Oct 26 2017, 16:58
dagurval updated this revision to Diff 1626.Nov 1 2017, 12:46
dagurval edited edge metadata.
dagurval marked 3 inline comments as done.
  • 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
dagurval added inline comments.Nov 1 2017, 12:47
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.

dagurval edited the summary of this revision. (Show Details)Nov 1 2017, 12:48
dagurval added a parent revision: D631: Move uint160S to header.
dagurval updated this revision to Diff 1627.Nov 1 2017, 13:03

Use braces

dagurval updated this revision to Diff 1636.Nov 3 2017, 09:06

Check that what we truncate is padding when decoding.

deadalnix requested changes to this revision.Nov 4 2017, 21:05

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

This revision now requires changes to proceed.Nov 4 2017, 21:05
dagurval updated this revision to Diff 1655.Nov 7 2017, 10:33
dagurval edited edge metadata.

Added test with example addresses. Added DstTypeChecker.

deadalnix requested changes to this revision.Nov 7 2017, 10:43
deadalnix added inline comments.
src/test/cashaddrenc_tests.cpp
50 ↗(On Diff #1655)

This doesn't looks like ti is used at all.

159 ↗(On Diff #1655)

remove empty line.

162 ↗(On Diff #1655)

This would benefit from C++11 style loop.

This revision now requires changes to proceed.Nov 7 2017, 10:43
dagurval added inline comments.Nov 7 2017, 11:45
src/test/cashaddrenc_tests.cpp
50 ↗(On Diff #1655)

It's used in test random_dst

162 ↗(On Diff #1655)

How would you read from 3 different vectors with a C++11 style loop?

dagurval updated this revision to Diff 1656.Nov 7 2017, 11:49
dagurval edited edge metadata.

remove empty line

deadalnix accepted this revision.Nov 7 2017, 15:25
deadalnix added inline comments.
src/test/cashaddrenc_tests.cpp
162 ↗(On Diff #1655)

Yes no, that was stupid.

This revision is now accepted and ready to land.Nov 7 2017, 15:25
This revision was automatically updated to reflect the committed changes.