Skip to content

Conversation

@mwb-al
Copy link
Contributor

@mwb-al mwb-al commented Jun 17, 2025

Description:

Updated getBalance method and related tests to accommodate parameter type change from string | null to string.

Related issue(s):

Fixes #3838

Checklist

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

…parameter instead of allowing null (hiero-ledger#3838)

Signed-off-by: Michał Walczak <[email protected]>
@mwb-al mwb-al requested review from a team as code owners June 17, 2025 10:21
@mwb-al mwb-al requested a review from Ferparishuertas June 17, 2025 10:21
@lfdt-bot
Copy link

lfdt-bot commented Jun 17, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

acuarica
acuarica previously approved these changes Jun 18, 2025
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

lgtm

quiet-node
quiet-node previously approved these changes Jun 20, 2025
Copy link
Contributor

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM looks like unit tests need some updates as well

@codecov
Copy link

codecov bot commented Jun 25, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...rvices/ethService/accountService/AccountService.ts 0.00% 1 Missing ⚠️
@@           Coverage Diff           @@
##             main    #3859   +/-   ##
=======================================
  Coverage        ?   41.29%           
=======================================
  Files           ?       83           
  Lines           ?     4770           
  Branches        ?      970           
=======================================
  Hits            ?     1970           
  Misses          ?     2536           
  Partials        ?      264           
Files with missing lines Coverage Δ
packages/relay/src/lib/eth.ts 49.11% <100.00%> (ø)
...rvices/ethService/accountService/AccountService.ts 19.29% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mwb-al mwb-al dismissed stale reviews from acuarica and quiet-node via 6b8671c June 26, 2025 11:08
@mwb-al mwb-al requested a review from Ferparishuertas June 26, 2025 11:38
@simzzz simzzz self-requested a review June 26, 2025 13:54
simzzz
simzzz previously approved these changes Jun 26, 2025
Copy link
Contributor

@simzzz simzzz left a comment

Choose a reason for hiding this comment

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

LGTM

acuarica
acuarica previously approved these changes Jun 26, 2025
@mwb-al mwb-al dismissed stale reviews from acuarica and simzzz via 02b7f3c July 1, 2025 09:33
quiet-node
quiet-node previously approved these changes Jul 14, 2025
Copy link
Contributor

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM

acuarica
acuarica previously approved these changes Jul 14, 2025
@acuarica acuarica removed this from the 0.70.0 milestone Jul 21, 2025
@acuarica acuarica added this to the 0.71.0 milestone Jul 21, 2025
@mwb-al
Copy link
Contributor Author

mwb-al commented Jul 22, 2025

@quiet-node @acuarica
Should we cache the result of eth_getBalance when using the latest block tag?
Here's a test that uses latest.

I noticed that latest is currently listed among the block tags for which caching is skipped, see list

I need to know which approach we want to take so I can get the tests to pass successfully.

@acuarica
Copy link
Contributor

acuarica commented Jul 22, 2025

Should we cache the result of eth_getBalance when using the latest block tag?

No, latest shouldn't be cached. I believe it's safe to remove this test case.

To recap, null is not a valid input for eth_getBalance. However, the mentioned test uses null to test the method internally. In this scenario the cache will be used.

@mwb-al mwb-al dismissed stale reviews from acuarica and quiet-node via d7cad5f July 23, 2025 09:49
Copy link
Contributor

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM Great work!

@arianejasuwienas arianejasuwienas merged commit 4a5454d into hiero-ledger:main Jul 24, 2025
73 of 79 checks passed
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.

Align eth_getBalance parameter validation with Ethereum's OpenRPC specification

7 participants