Remove unnecessary null terminator

[10gic]

Description

No need to append size number of '\0' in function TWStringCreateWithRawBytes. TWStringSize would output 2 * size without this fix, I think this is unnecessary.

TWString *_Nonnull TWStringCreateWithUTF8Bytes(const char *_Nonnull bytes) {
    auto s = new std::string(bytes);
    return s;
}

TWString *_Nonnull TWStringCreateWithRawBytes(const uint8_t *_Nonnull bytes, size_t size) {
    auto s = new std::string(bytes, bytes + size);
    // append null terminator
    s->append(size, '\0');                     // Don't need this
    return s;
}

Just like TWStringCreateWithUTF8Bytes, we don't append '\0'. We also no need to append '\0' in function TWStringCreateWithRawBytes.

Testing instructions

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Prefix PR title with [WIP] if necessary.
• Add tests to cover changes as needed.
• Update documentation as needed.
• I have read the guidelines for adding a new blockchain
• If there is a related Issue, mention it in the description (e.g. Fixes #<issue_number> ).

[codecov]

Codecov Report

Merging #1360 (5bb470a) into master (fb9ce77) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 5bb470a differs from pull request most recent head a52f5f9. Consider uploading reports for the commit a52f5f9 to get more accurate results

@@            Coverage Diff             @@
##           master    #1360      +/-   ##
==========================================
- Coverage   95.41%   95.40%   -0.02%     
==========================================
  Files         443      443              
  Lines       13402    13349      -53     
==========================================
- Hits        12787    12735      -52     
+ Misses        615      614       -1     

Impacted Files	Coverage Δ
src/interface/TWString.cpp	86.36% <100.00%> (-0.60%)	⬇️
src/Bitcoin/CashAddress.cpp	82.22% <0.00%> (-2.40%)	⬇️
src/interface/TWStoredKey.cpp	91.57% <0.00%> (-1.06%)	⬇️
src/NEO/Witness.h	85.71% <0.00%> (-0.96%)	⬇️
src/NEO/Address.cpp	77.77% <0.00%> (-0.80%)	⬇️
src/NEO/ReadData.cpp	76.66% <0.00%> (-0.76%)	⬇️
src/Algorand/Address.cpp	87.09% <0.00%> (-0.41%)	⬇️
src/Stellar/Address.cpp	88.88% <0.00%> (-0.31%)	⬇️
src/Nimiq/Address.cpp	91.30% <0.00%> (-0.25%)	⬇️
src/EOS/Asset.cpp	84.61% <0.00%> (-0.24%)	⬇️
... and 29 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb9ce77...a52f5f9. Read the comment docs.

[hewigovens]

This one seems only be used in tests, can you check if we can remove it? thanks!

[optout21]

std::string is not null-terminated, probably not needed, maybe it was added as a safety net for the case if std::string contents is used as C string.

[10gic]

std::string is not null-terminated, probably not needed, maybe it was added as a safety net for the case if std::string contents is used as C string.

I don't think so, some reasons:

1. We add size number of '\0', not just one '\0'.
2. If we need one '\0' here, we also need this in TWStringCreateWithUTF8Bytes. TWString should have consistent semantics.
3. We provide another function TWStringEqual(lhs, rhs) to test two TWString. It would be failed if lhs is created by TWStringCreateWithUTF8Bytes, and rhs is created by TWStringCreateWithRawBytes. Very strange behaviour.
4. We add it just for fixing unstable test case, see Fix zilliqa address data tests unstable #826

[10gic]

This one seems only be used in tests, can you check if we can remove it? thanks!

Suggest you don't remove this:

1. We already make this as a public interface, removing it may breaking downstream products.
2. TWStringCreateWithRawBytes is more flexible than TWStringCreateWithUTF8Bytes. Function TWStringCreateWithUTF8Bytes require null-terminated input.

[optout21]

Std::string can be initialized with null-terminated string, or with pointer and explicit size, both are fine.
When treating contents as C string, c_str() has to be used to have a guaranteed null-terminated string.

In TWStringUTF8Bytes, we use .data(), which is not correct at first sight, this is the real problem. Correct should be .c_str(). Should be changed definitely.

However, depending on implementation, data() may return null terminated always. In fact, what I found:
"Extra: In C++11 onwards, both functions are required to be the same. i.e. data is now required to be null-terminated. According to cppreference: "The returned array is null-terminated, that is, data() and c_str() perform the same function."
https://stackoverflow.com/questions/194634/string-c-str-vs-data
I have checked in our C++ code, data() and c_str() returns the same pointer. This may not be necessarily true in ios/android as slightly different toolchain is used.

So it looks like data() is actually fine, but nonetheless it should be c_str(), and then there is no need to add null terminator ourselves.