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

Distinguish auto-approvals in synthetic CryptoTransfer transfer lists #6290

Merged
merged 7 commits into from Apr 28, 2023

Conversation

tinker-michaelj
Copy link
Collaborator

@tinker-michaelj tinker-michaelj commented Apr 27, 2023

Description:

  • Closes spender allowance does not decrease as allowance is spent #6262
  • Updates TransferPrecompile to produce a synthetic CryptoTransferTransactionBody with is_approval = true for each asset change that was automatically switched to use an approval/allowance.
  • Includes EET to verify this behavior using a new RecordStreamAssertion implementation.

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj tinker-michaelj requested a review from a team as a code owner April 27, 2023 21:39
@Nana-EC Nana-EC added the Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service. label Apr 27, 2023
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Looking good.
2 suggestions - 1 for readability of tests and the other a missing TransactionID field check

@github-actions
Copy link

github-actions bot commented Apr 27, 2023

Node: Unit Test Results

  1 338 files    1 338 suites   1h 12m 54s ⏱️
97 416 tests 97 408 ✔️ 8 💤 0
99 048 runs  99 040 ✔️ 8 💤 0

Results for commit d30e47d.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +91.11 🎉

Comparison is base (133622e) 0.00% compared to head (731e17b) 91.11%.

❗ Current head 731e17b differs from pull request most recent head d30e47d. Consider uploading reports for the commit d30e47d to get more accurate results

Additional details and impacted files
@@              Coverage Diff               @@
##             develop    #6290       +/-   ##
==============================================
+ Coverage           0   91.11%   +91.11%     
- Complexity         0    17069    +17069     
==============================================
  Files              0     1294     +1294     
  Lines              0    49012    +49012     
  Branches           0     4918     +4918     
==============================================
+ Hits               0    44656    +44656     
- Misses             0     3424     +3424     
- Partials           0      932      +932     
Impacted Files Coverage Δ
.../contracts/precompile/impl/TransferPrecompile.java 96.14% <100.00%> (ø)

... and 1293 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Apr 27, 2023

Node: Integration Test Results

    3 files      3 suites   15m 13s ⏱️
151 tests 150 ✔️ 0 💤 1
152 runs  151 ✔️ 0 💤 1

Results for commit d30e47d.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Apr 27, 2023

Node: E2E Test Results

    1 files      1 suites   17m 7s ⏱️
309 tests 309 ✔️ 0 💤 0
327 runs  327 ✔️ 0 💤 0

Results for commit d30e47d.

♻️ This comment has been updated with latest results.

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Nana-EC
Nana-EC previously approved these changes Apr 28, 2023
Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LGTM
One suggestion for adding a debit check if useful

Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

LGTM !

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks @tinker-michaelj

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@tinker-michaelj tinker-michaelj merged commit 5d90093 into develop Apr 28, 2023
9 of 11 checks passed
@tinker-michaelj tinker-michaelj deleted the 06262-set-auto-approvals-in-synth-xfer branch April 28, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spender allowance does not decrease as allowance is spent
4 participants