-
Notifications
You must be signed in to change notification settings - Fork 143
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
Reduce duplication in Smart Contract Service #908
Conversation
…ties Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
…gic syntaxChecks Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
Codecov Report
@@ Coverage Diff @@
## master #908 +/- ##
============================================
+ Coverage 82.17% 84.25% +2.08%
- Complexity 4690 4761 +71
============================================
Files 402 410 +8
Lines 14933 14761 -172
Branches 1590 1502 -88
============================================
+ Hits 12271 12437 +166
+ Misses 2187 1899 -288
+ Partials 475 425 -50
Continue to review full report at Codecov.
|
cryptoTransferNet += adjustment.getAmount(); | ||
} | ||
} | ||
return cryptoTransferNet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this line be return -cryptoTransferNet;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a CryptoTransfer
, we can directly compute the affect on the payer (for example, if payer 0.0.1234
sends 1_000 tinybar
, then the transferList
will have an AccountAmount
entry like { account: 0.0.1234, amount: -1_000 }
...so then we getcryptoTransferNet=-1000
directly).
For the other operation types, we need to multiply a quantity by -1 to get the cryptoTransferNet
, since in these cases there is some amount (gas
, an initialBalance
, or a sent
amount) that will be deducted from the payer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I realized that all amount in transfer list from this payer is negative. Thanks for the clarification.
...va/com/hedera/services/fees/calculation/contract/queries/ContractCallLocalResourceUsage.java
Show resolved
Hide resolved
hedera-node/src/main/java/com/hedera/services/grpc/controllers/ContractController.java
Outdated
Show resolved
Hide resolved
hedera-node/src/main/java/com/hedera/services/txns/network/FreezeTransitionLogic.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall.
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
@tinker-michaelj thanks for addressing these minot issues. I think it's good to merge into master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me in general. The test coverage increased significantly due to the refactoring, so great work! As pointed out by Codecov, there are some new lines that are not covered by tests, but those could definitely improved in a future PR. We still have all the smart contract integration tests from test-clients that were passing to give us confidence for this merge.
Signed-off-by: tinker-michaelj <michael.tinker@hedera.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approve for the resolved merge conflicts and fix for MerkleAccountTokens
deserialization.
Related issue(s):
Summary of the change:
SmartContractServiceImpl
with aContractsController
that delegates queries toAnswerService
implementations, and uses the standard submission flow for transactions.SmartContractRequestHandler
method as aTransitionLogic
; refactor all prechecks asTransitionLogic.syntaxCheck
implementations, hence removing duplication between legacy handlers andTransactionValidationUtils
/AwareProcessLogic
components.FreezeHandler
as aTransitionLogic
to eliminate legacy record mapping fromAwareProcessLogic
.External impacts:
ContractCallLocal
query can be slightly higher now, since the prior code computed gas and non-gas fees separately (hence rounding down twice in two separatetinycent -> tinybar
conversions); the refactored code only rounds down once.