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

test: Fix transferFailsWithIncorrectAmounts test #10214

Merged
merged 17 commits into from Dec 18, 2023

Conversation

stoyanov-st
Copy link
Contributor

@stoyanov-st stoyanov-st commented Dec 1, 2023

Description:
Added a missing check for the amounts to be transferred while executing the classic token transfer function.

Related issue(s):

Fixes #
#9396
Notes for reviewer:

Checklist

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

@stoyanov-st stoyanov-st added the Limechain Work planned for the LimeChain team label Dec 1, 2023
@stoyanov-st stoyanov-st self-assigned this Dec 1, 2023
@stoyanov-st stoyanov-st linked an issue Dec 1, 2023 that may be closed by this pull request
Copy link

github-actions bot commented Dec 1, 2023

Node: HAPI Test (Crypto) Results

211 tests  ±0   204 ✔️ ±0   18m 13s ⏱️ +32s
  22 suites ±0       7 💤 ±0 
  22 files   ±0       0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: Unit Test Results

    2 293 files  ±0      2 293 suites  ±0   54m 48s ⏱️ + 7m 22s
118 445 tests  - 1  118 411 ✔️  - 1  34 💤 ±0  0 ±0 
126 862 runs   - 1  126 828 ✔️  - 1  34 💤 ±0  0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: HAPI Test (Token) Results

189 tests  ±0   189 ✔️ ±0   18m 0s ⏱️ +43s
  13 suites ±0       0 💤 ±0 
  13 files   ±0       0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: HAPI Test (Misc) Results

420 tests  ±0   330 ✔️ +1   30m 48s ⏱️ +54s
  74 suites ±0     90 💤  - 1 
  74 files   ±0       0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: E2E Test Results

    1 files  ±    0      1 suites  ±0   24m 56s ⏱️ + 24m 56s
311 tests +310  311 ✔️ +311  0 💤 ±0  0  - 1 
333 runs  +332  333 ✔️ +333  0 💤 ±0  0  - 1 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

This pull request removes 1 and adds 311 tests. Note that renamed tests count towards both.
EndToEndTests ‑ initializationError
EndToEndTests ‑ ADDRESS_BOOK_CONTROLCanUpdateADDRESS_BOOK
EndToEndTests ‑ ADDRESS_BOOK_CONTROLCanUpdateNODE_DETAILS
EndToEndTests ‑ AccountsGetPayerRecordsIfSoConfigured
EndToEndTests ‑ Acct57CanMakeSmallChanges
EndToEndTests ‑ Acct57CantMakeLargeChanges
EndToEndTests ‑ AddingSignaturesToExecutedTxFails
EndToEndTests ‑ AddingSignaturesToExecutedTxFailsWithLongTermEnabled
EndToEndTests ‑ AddingSignaturesToNonExistingTxFails
EndToEndTests ‑ AddingSignaturesToNonExistingTxFailsWithLongTermEnabled
EndToEndTests ‑ AddressAliasIdFuzzing
…

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: HAPI Test (Time Consuming) Results

21 tests  ±0     9 ✔️ ±0   25m 22s ⏱️ -10s
  2 suites ±0   12 💤 ±0 
  2 files   ±0     0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: Integration Test Results

278 tests  ±0   278 ✔️ ±0   26m 57s ⏱️ +12s
    5 suites ±0       0 💤 ±0 
    5 files   ±0       0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 1, 2023

Node: HAPI Test (Smart Contract) Results

398 tests  ±0   366 ✔️ ±0   46m 56s ⏱️ + 2m 23s
  55 suites ±0     32 💤 ±0 
  55 files   ±0       0 ±0 

Results for commit 48a2a4a. ± Comparison against base commit 3a99e37.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (3a99e37) 63.01% compared to head (48a2a4a) 62.99%.
Report is 3 commits behind head on develop.

Files Patch % Lines
...ontracts/hts/transfer/ClassicTransfersDecoder.java 0.00% 5 Missing ⚠️
...emcontracts/hts/transfer/ClassicTransfersCall.java 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10214      +/-   ##
=============================================
- Coverage      63.01%   62.99%   -0.02%     
+ Complexity     30824    30816       -8     
=============================================
  Files           3363     3363              
  Lines         135414   135419       +5     
  Branches       14092    14091       -1     
=============================================
- Hits           85325    85305      -20     
- Misses         46730    46754      +24     
- Partials        3359     3360       +1     

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

@stoyanov-st stoyanov-st marked this pull request as ready for review December 7, 2023 11:57
MrValioBg
MrValioBg previously approved these changes Dec 7, 2023
Copy link
Contributor

@MrValioBg MrValioBg left a comment

Choose a reason for hiding this comment

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

LGTM!

netopyr
netopyr previously approved these changes Dec 11, 2023
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

We don't want to throw exceptions out of HtsCall implementations; we should instead simply return an appropriate haltResult() or revertResult() when a negative number is passed here.

Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
@stoyanov-st stoyanov-st dismissed stale reviews from netopyr and MrValioBg via 388f163 December 13, 2023 11:33
@stoyanov-st stoyanov-st force-pushed the 09396-fix-transfer-fails-with-incorrect-amounts branch from f47ab20 to 388f163 Compare December 13, 2023 11:33
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
…mounts

# Conflicts:
#	hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/leaky/LeakyContractTestsSuite.java
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

I think it's more general and readable to add a @Nullable ResponseCodeEnum preemptingFailureStatus to the ClassicTransfersCall constructor; and set it based on the 3rd argument being negative.

But this works for now, tyvm @stoyanov-st !

Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

Hmm actually this change has unintended consequences and breaks other specs.

I'd suggest adding the @Nullable ResponseCodeEnum preemptingFailureStatus.

Screen Shot 2023-12-14 at 9 56 34 AM

Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Signed-off-by: Stanimir Stoyanov <stanimir.stoyanov@limechain.tech>
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

Looks great @stoyanov-st , tyvm! 💯

@stoyanov-st stoyanov-st merged commit 58b3aa4 into develop Dec 18, 2023
37 of 38 checks passed
@stoyanov-st stoyanov-st deleted the 09396-fix-transfer-fails-with-incorrect-amounts branch December 18, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Limechain Work planned for the LimeChain team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix transferFailsWithIncorrectAmounts test
4 participants