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

fix: modified sendRawTransaction to also poll MN for transaction's validity for Timeout and ConnectionDropped error #3229

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

quiet-node
Copy link
Member

@quiet-node quiet-node commented Nov 6, 2024

Description:

  • Modifies EthImpl.sendRawTransaction() to poll the MN for the transaction's validity not only for transactions that were successfully submitted to the network but also in cases of Timeout and ConnectionDropped errors.
  • Removes the MN instance in SDKClient.
  • Fixes unit tests in eth_sendRawTransaction.spec.ts to align with the new update.

Related issue(s):

Fixes #3207

Notes for reviewer:

  • The primary focus is on the EthImpl.sendRawTransaction. Please refer to the code comments for a clearer understanding of the rationale behind the changes.
  • The sdkClient.ts was only modified to remove the MN instance and update a couple of log message and code comments

Checklist

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

@quiet-node quiet-node added the enhancement New feature or request label Nov 6, 2024
@quiet-node quiet-node added this to the 0.60.0 milestone Nov 6, 2024
@quiet-node quiet-node self-assigned this Nov 6, 2024
@quiet-node quiet-node linked an issue Nov 6, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Nov 6, 2024

Test Results

 19 files  ±  0  279 suites  +34   30m 44s ⏱️ -58s
608 tests  -   1  602 ✅  -   1  4 💤 ±0  2 ❌ ±0 
823 runs  +137  817 ✅ +138  4 💤  - 1  2 ❌ ±0 

For more details on these failures, see this check.

Results for commit 9f9f162. ± Comparison against base commit acc4538.

This pull request removes 2 and adds 1 tests. Note that renamed tests count towards both.
"before all" hook in "@precompile-calls Tests for eth_call with HTS" ‑ RPC Server Acceptance Tests Acceptance tests @precompile-calls Tests for eth_call with HTS "before all" hook in "@precompile-calls Tests for eth_call with HTS"
"before each" hook for "should execute "eth_getStorageAt" request to get old state with passing specific block" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before each" hook for "should execute "eth_getStorageAt" request to get old state with passing specific block"
"after all" hook in "@web-socket-batch-1 eth_call" ‑ RPC Server Acceptance Tests Acceptance tests @web-socket-batch-1 eth_call "after all" hook in "@web-socket-batch-1 eth_call"

♻️ This comment has been updated with latest results.

@quiet-node quiet-node force-pushed the 3207-fix-remove-usage-of-mn-client-in-sdk-client branch 2 times, most recently from cd97639 to dda4831 Compare November 6, 2024 23:37
…y for Timeout and ConnectionDropped error

Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
@quiet-node quiet-node force-pushed the 3207-fix-remove-usage-of-mn-client-in-sdk-client branch from dda4831 to b405e91 Compare November 6, 2024 23:42
Copy link
Contributor

@victor-yanev victor-yanev left a comment

Choose a reason for hiding this comment

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

Good job, I like where this PR is headed but I have some suggestion on how we can improve it further

packages/relay/src/lib/eth.ts Show resolved Hide resolved
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
@konstantinabl
Copy link
Collaborator

konstantinabl commented Nov 7, 2024

@quiet-node I like the idea to remove the MN client from the sdkClient, however I think we should only change whats in the catch block of the sendRawTransaction. Current changes are confusing and worsening the readability at least from my POV. We can maybe discuss and brainstorm a better way, where we only change the catch block, since we only want new logic when error is thrown.

@quiet-node
Copy link
Member Author

quiet-node commented Nov 7, 2024

@quiet-node I like the idea to remove the MN client from the sdkClient, however I think we should only change whats in the catch block of the sendRawTransaction. Current changes are confusing and worsening the readability at least from my POV. We can maybe discuss and brainstorm a better way, where we only change the catch block, since we only want new logic when error is thrown.

@konstantinabl
Thanks for the concern yeah it’s a valid one!
However, I believe modifying the current implementation of sendRawTransaction in this way is essential for integrating the new logic, while still supporting the previous functionality. Sticking to the old implementation just because it's less confusing could limit our ability to capture and handle errors robustly. This adjustment enables us to securely capture the correct errors, handle them effectively, and reuse the mirror node polling logic in a more logical and consistent way. While it may initially seem unfamiliar, I believe this modification actually streamlines the process rather than adding complexity, allowing the codebase to evolve and adapt to new requirements. That said, I’m definitely open to suggestions and would be happy to discuss any ideas that could further improve this approach!

Copy link

sonarcloud bot commented Nov 11, 2024

@quiet-node quiet-node merged commit 765ad27 into main Nov 11, 2024
44 of 45 checks passed
@quiet-node quiet-node deleted the 3207-fix-remove-usage-of-mn-client-in-sdk-client branch November 11, 2024 16:57
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.48%. Comparing base (4f0b2ea) to head (9f9f162).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
packages/relay/src/lib/eth.ts 91.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3229      +/-   ##
==========================================
+ Coverage   83.17%   83.48%   +0.31%     
==========================================
  Files          66       66              
  Lines        4314     4305       -9     
  Branches      843      846       +3     
==========================================
+ Hits         3588     3594       +6     
+ Misses        483      470      -13     
+ Partials      243      241       -2     
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 85.70% <92.59%> (+0.40%) ⬆️
server 83.52% <ø> (ø)
ws-server 36.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/relay/src/lib/clients/sdkClient.ts 58.59% <100.00%> (+1.27%) ⬆️
.../relay/src/lib/services/hapiService/hapiService.ts 78.26% <100.00%> (ø)
packages/relay/src/lib/eth.ts 82.94% <91.66%> (+0.67%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: remove usage of MN client in SDK client
3 participants