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

Externalize the generation of child records for view calls #9795

Merged
merged 233 commits into from Nov 21, 2023

Conversation

petreze
Copy link
Contributor

@petreze petreze commented Nov 9, 2023

Description:
Reduces code duplication and fixes several remaining details involved in externalizing child ContractCallResult's.

1️⃣ In hedera-app,

  • Wires up and uses ChildRecordFinalizer to set transfer lists in the records of dispatched transactions.
  • Removes ExchangeRateSet from all child record receipts (not just preceding).
  • Adds record builder getters for newTotalSupply and minted serial numbers (we need this to encode output from burn/mint HTS system contract calls).

2️⃣ In contract-service,

  • Adds SystemContractOperations#externalizePreemptedDispatch() method to let HtsCall implementations externalize dispatches that were pre-empted for reasons not enforced by handlers (e.g. to prevent transfers to system accounts).
  • Reduces duplicate code in externalized view call results by adding an isViewCall flag to FullResult and externalizing results in HtsSystemContract.
  • Adds a ContractCallRecordBuilder to FullResult so that HtsCall implementations can directly return their used ContractCallRecordBuilder.
    • Updates HtsSystemContract to add the implied synthetic ContractCallResult to such record builders.
  • Makes FullResult a top-level record for readability.
  • Adds HtsCall.allowsStaticFrame() and reverts in HtsSystemContract when the frame is static and this method returns false.
  • Updates GrantRevokeKycTranslator to match mono-service behavior of leaving INVALID_SIGNATURE response code untouched (instead of converting to INVALID_FULL_PREFIX_SIGNATURE_FOR_PRECOMPILE).
  • Fixes burn and mint return types (neither is just an int64 responseCode).
  • Fixes ClassicTransfersCall to:
    1. Revert or halt on unsupported behavior.
    2. Emit ERC-style log events on transferFrom() and transferFromNFT().
    3. Consolidate multiple tokenTransferList's with the same TokenID.

3️⃣ In token-service,

  • Fixes FinalizeParentRecordHandler to deduct child token transfers from the top-level record (unlike in mono-service these need to be explicitly deducted from parent).
  • Reverses order of modifying allowances and token balances in AdjustFungibleTokenChangesStep to match mono-service behavior.
  • Adds a ChildFinalizeContext as the base interface of FinalizeContext, with just what is needed to finalize a single child record.

4️⃣ In test-clients,

  • Adds record snapshots for CryptoTransferHTSSuite
  • Adds some helpers to make record assertions more flexible if needed.
  • Adds ACCEPTED_MONO_GAS_CALCULATION_DIFFERENCE snapshot matching mode to allow mod-service gas calculations to differ in edge cases.
  • Enables 55-ish @HapiTests.

➡️ Next steps:

  • Eliminate all duplicate code in externalized call results.
  • Remove type parameter from DispatchForResponseCodeHtsCall (can always just use ContractCallRecordBuilder).
  • Try to replace all non-transfer HtsCall implementations with DispatchForResponseCodeHtsCall instances where custom behavior is achieved via FailureCustomizer and OutputFn instead of standalone HtsCall implementations.
  • Remove blockValues, value, isStaticCall from HtsCallAttempt (these are now available on the MessageFrame passed to execute()).

Related issue(s):

Checklist

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

MiroslavGatsanoga and others added 30 commits October 26, 2023 16:40
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
method no-op

Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
#9569)

Signed-off-by: Stoyan Panayotov <stoyan.panayotov@limechain.tech>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Hendrik Ebbers <hendrik.ebbers@web.de>
Signed-off-by: Timo Brandstätter <timo@swirldslabs.com>
Co-authored-by: Timo Brandstätter <timo@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Austin Littley <austin@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Matt Hess <matt.hess@swirldslabs.com>
Co-authored-by: Matt Hess <matt.hess@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
… failures. (#9560)

Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
…9600)

Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Kelly Greco <kelly@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Cody Littley <cody@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Lazar Petrovic <lpetrovic05@gmail.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Lev Povolotsky <lev@swirldslabs.com>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
# Conflicts:
#	hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/scope/HandleSystemContractOperations.java
#	hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/scope/QuerySystemContractOperations.java
#	hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/hts/AbstractNonRevertibleTokenViewCall.java
#	hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/scope/QuerySystemContractOperationsTest.java
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>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
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.

LGTM

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.

Just a few questions. Not blocked by the questions. Token Service changes LGTM ! Thanks @tinker-michaelj @petreze !

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj tinker-michaelj dismissed stale reviews from Neeharika-Sompalli and themself via b6bfd63 November 20, 2023 17:57
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

Much simpler. Thanks for making the changes. A few minor comments but definitely non blocking

lukelee-sl
lukelee-sl previously approved these changes Nov 20, 2023
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

LGTM

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>
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

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

Comparison is base (791ab23) 0.00% compared to head (6bbcf2b) 65.34%.
Report is 6 commits behind head on develop.

Files Patch % Lines
...ontracts/hts/transfer/ClassicTransfersDecoder.java 0.00% 41 Missing ⚠️
...tracts/hts/transfer/TransferEventLoggingUtils.java 5.40% 35 Missing ⚠️
...ken/impl/handlers/FinalizeParentRecordHandler.java 31.25% 29 Missing and 4 partials ⚠️
...contracts/hts/transfer/Erc721TransferFromCall.java 28.57% 20 Missing ⚠️
...t/impl/exec/systemcontracts/HtsSystemContract.java 47.05% 12 Missing and 6 partials ⚠️
...systemcontracts/hts/create/ClassicCreatesCall.java 42.30% 11 Missing and 4 partials ⚠️
...emcontracts/hts/transfer/ClassicTransfersCall.java 50.00% 10 Missing and 2 partials ⚠️
...ontract/impl/exec/systemcontracts/hts/HtsCall.java 15.38% 11 Missing ⚠️
...stemcontracts/hts/transfer/Erc20TransfersCall.java 41.66% 7 Missing ⚠️
...rkflows/record/SingleTransactionRecordBuilder.java 0.00% 6 Missing ⚠️
... and 21 more
Additional details and impacted files
@@              Coverage Diff               @@
##             develop    #9795       +/-   ##
==============================================
+ Coverage           0   65.34%   +65.34%     
- Complexity         0    30219    +30219     
==============================================
  Files              0     3320     +3320     
  Lines              0   126812   +126812     
  Branches           0    13182    +13182     
==============================================
+ Hits               0    82863    +82863     
- Misses             0    40739    +40739     
- Partials           0     3210     +3210     

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

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.

LGTM

@tinker-michaelj tinker-michaelj merged commit 4a56cd0 into develop Nov 21, 2023
16 of 17 checks passed
@tinker-michaelj tinker-michaelj deleted the fix-generalize-externalization branch November 21, 2023 17:51
nickpoorman pushed a commit that referenced this pull request Nov 22, 2023
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Petar Tonev <petar.tonev@limechain.tech>
Signed-off-by: Stoyan Panayotov <stoyan.panayotov@limechain.tech>
Signed-off-by: Hendrik Ebbers <hendrik.ebbers@web.de>
Signed-off-by: Timo Brandstätter <timo@swirldslabs.com>
Signed-off-by: Austin Littley <austin@swirldslabs.com>
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Matt Hess <matt.hess@swirldslabs.com>
Signed-off-by: Ivan Malygin <ivan@swirldslabs.com>
Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Kelly Greco <kelly@swirldslabs.com>
Signed-off-by: Cody Littley <cody@swirldslabs.com>
Signed-off-by: Lazar Petrovic <lpetrovic05@gmail.com>
Signed-off-by: Lev Povolotsky <lev@swirldslabs.com>
Signed-off-by: Alexander Gadzhalov <alexander.gadzhalov@limechain.tech>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Co-authored-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Co-authored-by: Stoyan Panayotov <stoyan.panayotov@limechain.tech>
Co-authored-by: Hendrik Ebbers <hendrik.ebbers@web.de>
Co-authored-by: Timo Brandstätter <timo@swirldslabs.com>
Co-authored-by: Austin Littley <102969658+alittley@users.noreply.github.com>
Co-authored-by: Michael Tinker <michael.tinker@swirldslabs.com>
Co-authored-by: Matt Hess <matt.hess@swirldslabs.com>
Co-authored-by: Ivan Malygin <ivan@swirldslabs.com>
Co-authored-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
Co-authored-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Co-authored-by: Kelly Greco <82919061+poulok@users.noreply.github.com>
Co-authored-by: Cody Littley <56973212+cody-littley@users.noreply.github.com>
Co-authored-by: Lazar Petrovic <lpetrovic05@gmail.com>
Co-authored-by: Lev Povolotsky <16233475+povolev15@users.noreply.github.com>
Co-authored-by: Alexander Gadzhalov <alexander.gadzhalov@limechain.tech>
Co-authored-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Co-authored-by: Neeharika Sompalli <52669918+Neeharika-Sompalli@users.noreply.github.com>
Signed-off-by: Nick Poorman <nick@swirldslabs.com>
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