Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Changed the return type of removeAllSignatures method #2559

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

ivaylogarnev-limechain
Copy link
Contributor

Description:
This PR changes the return type of the removeAllSignatures() method in the Transaction class. Additionally, it addresses and fixes the related unit and integration tests to ensure compatibility with this change

Fixes
#2546

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

…ction class and fixed unit and integration related tests

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
…icKey method

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
@ivaylogarnev-limechain ivaylogarnev-limechain changed the title feat: Changed the return type of removeAllSignatures method of Transa… feat: Changed the return type of removeAllSignatures method Oct 3, 2024
Signed-off-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (6f0ae98) to head (e7e0546).

Files with missing lines Patch % Lines
src/transaction/Transaction.js 92.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6f0ae98) and HEAD (e7e0546). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (6f0ae98) HEAD (e7e0546)
3 2
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2559      +/-   ##
==========================================
- Coverage   84.50%   78.23%   -6.27%     
==========================================
  Files         283      283              
  Lines       71021    71035      +14     
==========================================
- Hits        60015    55574    -4441     
- Misses      11006    15461    +4455     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ivaylonikolov7 ivaylonikolov7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but considering that the codecov fails... Can we add unit tests for L1920 and L1928 for Transaction.js. I think if we keep adding unit tests at some point it's going to stop crying that it doesn't reach the threshold.

It's something I tried to do in my PendingAirdropId PR. I tried to reach 100% codecov. It doesn't fail consistently for some reason but I think its because we are on the edge of the threshold and this is why it sometimes fails and sometimes passes.

…licKey method

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
ivaylonikolov7
ivaylonikolov7 previously approved these changes Oct 9, 2024
Copy link
Contributor

@ivaylonikolov7 ivaylonikolov7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ivaylogarnev-limechain ivaylogarnev-limechain force-pushed the feat/2546-remove-all-signatures-return-type branch 2 times, most recently from 6ce791d to 1fc4373 Compare October 9, 2024 10:13
@ivaylogarnev-limechain ivaylogarnev-limechain force-pushed the feat/2546-remove-all-signatures-return-type branch from e7e0546 to 5136a64 Compare October 9, 2024 10:53
Copy link

sonarcloud bot commented Oct 9, 2024

Copy link
Contributor

@agadzhalov agadzhalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ivaylonikolov7 ivaylonikolov7 merged commit 10dfd3f into main Oct 10, 2024
18 checks passed
@ivaylonikolov7 ivaylonikolov7 deleted the feat/2546-remove-all-signatures-return-type branch October 10, 2024 18:27
b-l-u-e pushed a commit to b-l-u-e/hedera-sdk-js that referenced this pull request Oct 13, 2024
…h#2559)

* feat: Changed the return type of removeAllSignatures method of Transaction class and fixed unit and integration related tests

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>

* fix: Fixed some CI comments for typezation in collectSignaturesByPublicKey method

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>

* tests: Added tests for early return checks in _coolectSignaturesByPublicKey method

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>

---------

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
Signed-off-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
Co-authored-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>
b-l-u-e pushed a commit to b-l-u-e/hedera-sdk-js that referenced this pull request Oct 13, 2024
…h#2559)

* feat: Changed the return type of removeAllSignatures method of Transaction class and fixed unit and integration related tests

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>

* fix: Fixed some CI comments for typezation in collectSignaturesByPublicKey method

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>

* tests: Added tests for early return checks in _coolectSignaturesByPublicKey method

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>

---------

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
Signed-off-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
Co-authored-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants