feat: add remove signature methods for Transaction and related unit tests#1218
Conversation
Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
…ctions in TransactionUnitTests Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
|
Hey, sorry to say, but I'm honestly unsure about how to deal with the higher than desired cyclomatic compliexity and length of the functions. I hope its okay to ask about a direction with those issues. |
|
Hey @Kubikusik 👋 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 #771 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for working on this @Kubikusik! The implementation is solid - both methods are functionally correct, the test coverage is thorough, and you've handled the internal state management (clearing mTransactions, removing stale auto-generated signatures) thoughtfully. Regarding your question about cyclomatic complexity - of course it's okay to ask! I left a comment on the code with a concrete suggestion. The main thing is to extract the duplicated signature byte extraction logic into a shared helper function, which should bring the complexity and length down for both methods. Once that's addressed, we should be good to go! Let me know if you have any questions.
784474a to
30a86a3
Compare
Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
30a86a3 to
483ff7b
Compare
|
Ok, i think i have implemented the proposed solution. Please, do inform me of any more potential improvements. (Its a great way for me to learn after all). |
rwalworth
left a comment
There was a problem hiding this comment.
Nice work on the helper extraction @Kubikusik, the code reads much cleaner now! Since you asked about more potential improvements, I left a few suggestions below for additional test cases that would strengthen the coverage. These aren't blocking (the existing tests cover all the cases from the issue) but they'd exercise some important code paths that aren't covered yet. Let me know what you think!
|
So, I added the mentioned tests, it also showed that i forgot to clear sigmap if sigpairs is empty (2 bytes discrepancy), so fixed that (good test idea, thanks!) |
…map in removeSignature Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
89c9288 to
2792fd9
Compare
rwalworth
left a comment
There was a problem hiding this comment.
Nice work @Kubikusik, the additional tests you added caught a real bug with the empty sigmap clearing, which is exactly the kind of thing these edge-case tests are meant to surface! The implementation is correct, well-tested, and covers all the requirements from the issue. One small thing left to fix and then I'll run the workflows and merge!
There was a problem hiding this comment.
LGTM @Kubikusik - the implementation is solid, the tests are thorough, and you've addressed every piece of feedback along the way. Running the workflows and merging, thank you for the great work on this! 🎉
rwalworth
left a comment
There was a problem hiding this comment.
LGTM @Kubikusik! The implementation is correct, the test coverage is excellent, and you've addressed all the feedback cleanly through each iteration. I'll run the workflows and merge!
Duplicate review — see earlier approval.
rwalworth
left a comment
There was a problem hiding this comment.
Actually, it looks like the Code / Lint (sdk, src/sdk/main) check is failing because of a formatting change to an unrelated declaration in Transaction.h. It looks like the determineStatus line wrapping was accidentally reformatted. Please revert it to the original formatting so clang-format-17 is happy.
Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
a2591b0 to
762298e
Compare
Description:
Add support for removing specific or all signatures from a frozen Transaction.
Related issue(s):
Fixes #771
Notes for reviewer:
When a signature is removed, explicitly clears mTransactions and removes "stale" auto-generated signatures from the sigmap.
This ensures that the next time getTransactionSize() or makeRequest() is called, the SDK cleanly regenerates signatures for the remaining keys instead of creating duplicate entries in the signature map.
Tested with ED25519 keys, also added separate test for ECDSAsecp256k1 keys' case.
Checklist