Task #3886
closedRemove util::Digest class template
100%
Description
ndn::util::Digest
has been superseded by ndn::security::transform::DigestFilter
.
Updated by Davide Pesavento almost 8 years ago
- Assignee deleted (
Davide Pesavento)
Updated by Alex Afanasyev almost 8 years ago
I disagree with deprecation of this class. DigestFilter
is not a replacement of it, but the underlying mechanism that should be used for digest calculations.
I agree that ndn-cxx's util/
folder became a kitchen sink for various unrelated things, but unless we create a convenient framework for a better place for those, I would like to keep Digest (Sha256) class.
Updated by Alex Afanasyev almost 8 years ago
- Status changed from New to Rejected
Updated by Davide Pesavento almost 8 years ago
DigestFilter is not a replacement of it, but the underlying mechanism that should be used for digest calculations.
So you agree that util::Digest
should be reimplemented using transform::DigestFilter
then? Main motivation is getting rid of cryptopp dependency.
Updated by Alex Afanasyev almost 8 years ago
Yes, it definitely should be re-implemented without cryptopp dependency. The interface will need to change a bit (right now, the template parameter is cryptopp-type), but that should be fine given only two known projects are using this type and probably using Sha256 typedef.
Updated by Davide Pesavento almost 8 years ago
Apparently we have a THIRD way of calculating a SHA256 digest in ndn-cxx: the computeSha256Digest()
function in util/crypto.hpp
(basically the only function in that file). Are you going to tell me we need all three of them?
Updated by Alex Afanasyev almost 8 years ago
It is not a third way. It is a helper to calculate digest, which was inteoduced long time ago. If you're suggesting yhag writing 2-4 lines of code using "transformation" API is the way of calculatig digest in all cases, I will strongly disagree. The transformation API was/is simply a way to avoid cryptopp dependency and rigidity of openssl. It is a more or less internal business of the library. What is so special in having a number of helper APIs that don't make assumptions about specific APIs and shortcut code writing?
Updated by Davide Pesavento almost 8 years ago
I understand your point about the transform API, and I could even agree: transform is a medium-level API that requires a couple of lines of code and the usage of streams or buffers. You want something higher level, easier to use for simple cases, and more compact to write. Fine. We have computeSha256Digest
for that, it's stateless and trivial to use. I'm not suggesting to remove it.
But then what do we need the util::Digest
class for? You said transform::DigestFilter
is not a replacement for it, but I don't understand why. From an API perspective they're very similar and require more or less the same number of lines of code...
Updated by Junxiao Shi almost 8 years ago
- Status changed from Rejected to New
- Assignee set to Davide Pesavento
20170206 call agrees that ndn::util::Digest
can be deleted (not deprecated) because it relies on CryptoPP.
Deprecation notice is NOT needed because a breaking change notice is already included in v0.5.1 release notes.
ndn::util::Sha256
should be re-implemented with DigestFilter
with its public API unchanged, because it's used by ChronoSync.
Updated by Junxiao Shi almost 8 years ago
- Blocks Task #3946: Get rid of cryptopp dependency added
Updated by Davide Pesavento over 7 years ago
- Subject changed from Deprecate Digest class template to Remove util::Digest class template
Updated by Davide Pesavento over 7 years ago
- Blocked by Task #3924: Reimplement util::Sha256 without cryptopp added
Updated by Davide Pesavento over 7 years ago
- Status changed from New to In Progress
- Estimated time deleted (
1.50 h)
Updated by Davide Pesavento over 7 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
https://gerrit.named-data.net/4018
Now, should util/digest.hpp
be renamed and/or moved somewhere else? For example:
util/sha256.hpp
security/sha256.hpp
security/crypto-hash-helpers.hpp
(inspired bysigning-helpers.hpp
; the more general name allows easier additions in the future)
Note that we also have a security/digest-sha256.hpp
which can cause some confusion...
Updated by Davide Pesavento over 7 years ago
- Status changed from Code review to Closed
Updated by Davide Pesavento over 7 years ago
Davide Pesavento wrote:
Now, should
util/digest.hpp
be renamed and/or moved somewhere else? For example:
util/sha256.hpp
security/sha256.hpp
security/crypto-hash-helpers.hpp
(inspired bysigning-helpers.hpp
; the more general name allows easier additions in the future)Note that we also have a
security/digest-sha256.hpp
which can cause some confusion...
Actually, I'm still waiting for an answer to the above.
Updated by Junxiao Shi over 7 years ago
If your goal is compatibility, as evidenced by refusing to deprecate duplicate API or improve Doxygen, don't rename anything.
Updated by Davide Pesavento over 7 years ago
I didn't mean it as a breaking change. We move the header and leave a deprecated "forwarding header" in its place, like it was done for mgmt/nfd/* headers.