feat: token transfer operator == adition#1320
feat: token transfer operator == adition#1320Egbaiyelo wants to merge 5 commits intohiero-ledger:mainfrom
Conversation
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
|
Hey @Egbaiyelo 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #1298 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
TIP This summary will be updated as you push new changes. Give us feedback
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for the contribution @Egbaiyelo! There are a few things to address before this can be merged:
Issue link fix: The PR description has Fixes # 1298 (space after #) - GitHub won't auto-link that, which is why the bot flagged it. Please update it to Fixes #1298 (no space).
I left a few comments below on the code changes. Let me know if you have any questions!
src/sdk/main/include/TokenTransfer.h
Outdated
| /** | ||
| * Compare this TokenTransfer instance to another TokenTransfer instance and determine if they represent the same pending TokenTransfer. | ||
| * | ||
| * @param other The other TokenTransfer with which to compare this TokenTransfer. | ||
| * @return \c TRUE if this TokenTransfer is the same as the input TokenTransfer, otherwise \c FALSE. | ||
| */ | ||
| friend bool operator==(const TokenTransfer&, const TokenTransfer&); |
There was a problem hiding this comment.
issue (blocking): The operator== is declared twice - once inside the class as friend (without [[nodiscard]]) and once as a free function outside the class (with [[nodiscard]]). Neither form is quite right.
Since all members of TokenTransfer are public, friend isn't needed. The rest of the codebase uses a member function for operator== (see PendingAirdropId, NftId). Please use that pattern instead:
// In the header, inside the class:
[[nodiscard]] bool operator==(const TokenTransfer& other) const;Remove the friend declaration inside the class and the entire block outside the class (the // Adding nodiscard... comment, the duplicate Doxygen block, and the [[nodiscard]] bool operator== line).
In TokenTransfer.cc, update the definition accordingly:
bool TokenTransfer::operator==(const TokenTransfer& other) const
{
return (mTokenId == other.mTokenId) && (mAccountId == other.mAccountId) &&
(mAmount == other.mAmount) && (mIsApproval == other.mIsApproval) &&
(mExpectedDecimals == other.mExpectedDecimals);
}There was a problem hiding this comment.
My bad though it did say friend in the issue, it said add [[nodiscard]] friend and i saw it was never like that in the codebase and I couldn't even do it like that cause of lint #1298
There was a problem hiding this comment.
That makes sense - the issue description was a bit ambiguous there. Removing friend was the right call.
Two things still needed on the declaration:
constqualifier — since this comparison doesn't modify*this, the method should beconst:[[nodiscard]] bool operator==(const TokenTransfer& other) const;
- Parameter name - minor, but the codebase convention names the parameter in the declaration too (see
PendingAirdropId.h).
Also, the implementation in TokenTransfer.cc still needs to be updated - I left a comment there with the details.
|
I genuinely cant run the tests for some reason? something about hproto-populate? |
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
1d2345c to
b79455b
Compare
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
|
@rwalworth So I am a bit confused with the test cases, i came back realising I might not have done the right thing, I think they cover everything now but I dont see the structure of what I did in the codebase |
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for the updates @Egbaiyelo — good progress on the SPDX header fix and adding the //----- separator! There are still a couple of things to address before this can merge, the most critical being that the operator== implementation in TokenTransfer.cc is a free function rather than a member function, which means it won't compile. I've left the details in the comments below.
| //----- | ||
| bool operator==(const TokenTransfer& other) { | ||
| return (mTokenId == other.mTokenId) && (mAccountId == other.mAccountId) && | ||
| (mAmount == other.mAmount) && (mIsApproval == other.mIsApproval) && | ||
| (mExpectedDecimals == other.mExpectedDecimals); | ||
| } |
There was a problem hiding this comment.
issue (blocking): The implementation is a free function, not a member function of TokenTransfer.
As written, bool operator==(const TokenTransfer& other) is a free function trying to access mTokenId, mAccountId, etc. directly - those are members of TokenTransfer, not variables in scope, so this won't compile.
Please use the member function form with the TokenTransfer:: scope qualifier and const at the end, matching the style of other functions in this file:
bool TokenTransfer::operator==(const TokenTransfer& other) const
{
return (mTokenId == other.mTokenId) && (mAccountId == other.mAccountId) &&
(mAmount == other.mAmount) && (mIsApproval == other.mIsApproval) &&
(mExpectedDecimals == other.mExpectedDecimals);
}Also note: the opening brace goes on its own line to match the file's style.
There was a problem hiding this comment.
noted, just realizing the free function thing.
Why is it a free function though? it's in the body of TokenTransfer
There was a problem hiding this comment.
In a .cc file, bool operator==(const TokenTransfer& other) without the TokenTransfer:: prefix IS a free function - even though it's typed inside the same file as the class definition, the .cc file has no class body, so there's no implicit class scope. The ClassName:: qualifier is what ties it to the class.
Looking at the latest commit, the definition now reads bool TokenTransfer::operator==(const TokenTransfer& other) const - that's the correct member function form. You got it!
src/sdk/main/src/TokenTransfer.cc
Outdated
| return (mTokenId == other.mTokenId) && (mAccountId == other.mAccountId) && | ||
| (mAmount == other.mAmount) && (mIsApproval == other.mIsApproval) && |
There was a problem hiding this comment.
todo (blocking): There is still trailing whitespace at the end of these two lines (a space before the line break after &&).
You can catch these with clang-format-17 -i src/sdk/main/src/TokenTransfer.cc, or most editors can highlight and trim trailing whitespace automatically.
There was a problem hiding this comment.
I think ill add git hooks to my local repo to solve this white space thing
There was a problem hiding this comment.
That's a great idea! A pre-commit hook running clang-format-17 -i on staged files would catch this automatically. The latest commit looks clean - no trailing whitespace.
The tests look good - they cover all three cases required by the issue:
The structure (going straight from One minor thing: in The reason you can't run the tests right now is the compilation error in |
Can you share the full error message? An error mentioning "hproto-populate" sounds like a build system or dependency issue unrelated to your changes - possibly a missing proto generation step or a stale build cache. |
| TokenTransfer diffTestAccountId(getTestTokenId(), AccountId(12345ULL), getTestAmount(), getTestExpectedDecimals(), getTestIsApproval()); | ||
| TokenTransfer diffTestAmount(getTestTokenId(), getTestAccountId(), 500LL, getTestExpectedDecimals(), getTestIsApproval()); | ||
| TokenTransfer diffTestExpectedDecimals(getTestTokenId(), getTestAccountId(), getTestAmount(), 2U, getTestIsApproval()); | ||
| TokenTransfer diffTestIsApproval(getTestTokenId(), getTestAccountId(), getTestAmount(), getTestExpectedDecimals(), false); |
There was a problem hiding this comment.
Just hoping this is the appropriate way to space this
There was a problem hiding this comment.
suggestion (non-blocking): The column-aligned variable declarations here (extra spaces padding variable names to align) and the space inside ( TokenId(...) won't survive clang-format-17. The .clang-format config has AlignConsecutiveDeclarations: None and SpacesInParentheses: false.
The lint check only runs on src/sdk/main/, so this won't fail CI - but it's worth running clang-format on the test file too to keep things consistent:
clang-format-17 -i src/sdk/tests/unit/TokenTransferUnitTests.ccNot a blocker - but something to consider.
|
ill rebuild it actually |
Signed-off-by: Egbaiyelo <moteniolaegbaiyelo@trentu.ca>
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for sticking with this @Egbaiyelo - the implementation and test coverage are looking good now! The operator== logic is correct, the //----- separator is there, the member function form is right, and the SPDX header is clean. Just one small doc comment fix needed in the header before this can merge.
| /** | ||
| * Compare this TokenTransfer instance to another TokenTransfer instance and determine if they represent the same pending TokenTransfer. | ||
| * | ||
| * @param other The other TokenTransfer with which to compare this TokenTransfer. | ||
| * @return \c TRUE if this TokenTransfer is the same as the input TokenTransfer, otherwise \c FALSE. | ||
| */ | ||
| [[nodiscard]] bool operator==(const TokenTransfer& other) const; | ||
|
|
There was a problem hiding this comment.
todo (blocking): The doc comment says "determine if they represent the same pending TokenTransfer" - the word "pending" was carried over from PendingAirdropId's comment but doesn't apply here. TokenTransfer is not a "pending" transfer.
Please change it to:
/**
* Compare this TokenTransfer to another TokenTransfer and determine if they represent the same TokenTransfer.
*
* @param other The other TokenTransfer with which to compare this TokenTransfer.
* @return \c TRUE if this TokenTransfer is the same as the input TokenTransfer, otherwise \c FALSE.
*/Also note: the comment line currently exceeds the 120-character column limit, so clang-format will reflow it - the shorter version above fits cleanly.
| TokenTransfer diffTestAccountId(getTestTokenId(), AccountId(12345ULL), getTestAmount(), getTestExpectedDecimals(), getTestIsApproval()); | ||
| TokenTransfer diffTestAmount(getTestTokenId(), getTestAccountId(), 500LL, getTestExpectedDecimals(), getTestIsApproval()); | ||
| TokenTransfer diffTestExpectedDecimals(getTestTokenId(), getTestAccountId(), getTestAmount(), 2U, getTestIsApproval()); | ||
| TokenTransfer diffTestIsApproval(getTestTokenId(), getTestAccountId(), getTestAmount(), getTestExpectedDecimals(), false); |
There was a problem hiding this comment.
suggestion (non-blocking): The column-aligned variable declarations here (extra spaces padding variable names to align) and the space inside ( TokenId(...) won't survive clang-format-17. The .clang-format config has AlignConsecutiveDeclarations: None and SpacesInParentheses: false.
The lint check only runs on src/sdk/main/, so this won't fail CI - but it's worth running clang-format on the test file too to keep things consistent:
clang-format-17 -i src/sdk/tests/unit/TokenTransferUnitTests.ccNot a blocker - but something to consider.
| //----- | ||
| bool operator==(const TokenTransfer& other) { | ||
| return (mTokenId == other.mTokenId) && (mAccountId == other.mAccountId) && | ||
| (mAmount == other.mAmount) && (mIsApproval == other.mIsApproval) && | ||
| (mExpectedDecimals == other.mExpectedDecimals); | ||
| } |
There was a problem hiding this comment.
In a .cc file, bool operator==(const TokenTransfer& other) without the TokenTransfer:: prefix IS a free function - even though it's typed inside the same file as the class definition, the .cc file has no class body, so there's no implicit class scope. The ClassName:: qualifier is what ties it to the class.
Looking at the latest commit, the definition now reads bool TokenTransfer::operator==(const TokenTransfer& other) const - that's the correct member function form. You got it!
src/sdk/main/src/TokenTransfer.cc
Outdated
| return (mTokenId == other.mTokenId) && (mAccountId == other.mAccountId) && | ||
| (mAmount == other.mAmount) && (mIsApproval == other.mIsApproval) && |
There was a problem hiding this comment.
That's a great idea! A pre-commit hook running clang-format-17 -i on staged files would catch this automatically. The latest commit looks clean - no trailing whitespace.
Description:
Implementing the TokenTransfer== operator
Related issue(s):
Fixes #1298
Notes for reviewer:
Checklist