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

Adjust the key gathering context methods to translate exceptions from preHandle #9841

Merged
merged 1 commit into from Nov 16, 2023

Conversation

jsync-swirlds
Copy link
Member

@jsync-swirlds jsync-swirlds commented Nov 10, 2023

Adjust the key gathering context methods to translate exceptions from preHandle; key gathering should not fail for non-pure checks.

  • Modified PreHandleContextImpl to translate exceptions from PreHandle in allKeysForTransaction
  • Modified HandleContextImpl to translate exceptions from PreHandle in allKeysForTransaction
  • Modified tests to match

Note: These changes assume that services preHandle methods only fail for situations where keys cannot be gathered. To be fully correct we will need to modify a few service handlers that do not meet that expectation.

@jsync-swirlds jsync-swirlds self-assigned this Nov 10, 2023
@jsync-swirlds jsync-swirlds linked an issue Nov 10, 2023 that may be closed by this pull request
Copy link

github-actions bot commented Nov 10, 2023

Node: Unit Test Results

    2 280 files  ±0      2 280 suites  ±0   1h 26m 33s ⏱️ - 2m 5s
118 339 tests ±0  118 305 ✔️ ±0  34 💤 ±0  0 ±0 
126 657 runs  ±0  126 623 ✔️ ±0  34 💤 ±0  0 ±0 

Results for commit 0227c86. ± Comparison against base commit 649ea48.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 10, 2023

Node: E2E Test Results

    1 files  ±    0      1 suites  ±0   21m 5s ⏱️ + 21m 5s
312 tests +311  312 ✔️ +312  0 💤 ±0  0  - 1 
334 runs  +333  334 ✔️ +334  0 💤 ±0  0  - 1 

Results for commit 0227c86. ± Comparison against base commit 649ea48.

This pull request removes 1 and adds 312 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 Nov 10, 2023

Node: Integration Test Results

280 tests  ±0   280 ✔️ ±0   28m 1s ⏱️ -35s
    5 suites ±0       0 💤 ±0 
    5 files   ±0       0 ±0 

Results for commit 0227c86. ± Comparison against base commit 649ea48.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 11, 2023

Node: HAPI Test Results

1 246 tests  ±0   885 ✔️ ±0   1h 28m 58s ⏱️ + 3m 54s
   166 suites ±0   361 💤 ±0 
   166 files   ±0       0 ±0 

Results for commit 0227c86. ± Comparison against base commit 649ea48.

♻️ This comment has been updated with latest results.

@jsync-swirlds jsync-swirlds marked this pull request as ready for review November 13, 2023 17:50
@jsync-swirlds jsync-swirlds requested a review from a team as a code owner November 13, 2023 17:50
@jsync-swirlds jsync-swirlds force-pushed the 09838-fix-key-gathering branch 3 times, most recently from ba76ac4 to f524433 Compare November 14, 2023 21:05
…preHandle, as key gathering should not fail for non-pure checks.

* Modified PreHandleContextImpl to suppress exceptions from PreHandle in `allKeysForTransaction'
* Modified HandleContextImpl to suppress exceptions from PreHandle in `allKeysForTransaction'
* Modified tests to match

Note: These changes assume that services preHandle methods set required keys as early as possible, before making any state-based validations.  To be fully correct we will need to modify a few service handlers that do not meet that expectation.

Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
@jsync-swirlds jsync-swirlds changed the title Adjust the key gathering context methods to suppress exceptions from preHandle Adjust the key gathering context methods to translate exceptions from preHandle Nov 16, 2023
@jsync-swirlds jsync-swirlds merged commit 8946316 into develop Nov 16, 2023
14 of 15 checks passed
@jsync-swirlds jsync-swirlds deleted the 09838-fix-key-gathering branch November 16, 2023 16:22
nickpoorman pushed a commit that referenced this pull request Nov 22, 2023
… preHandle (#9841)

Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify workflow to translate exceptions in allKeysForTransaction
3 participants