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.

Sign up for GitHub

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

Jump to bottom

Skip account balance check for eth_call #1949

Merged
merged 13 commits into from
Oct 4, 2023
Merged

Skip account balance check for eth_call #1949

merged 13 commits into from
Oct 4, 2023

Conversation

begmaroman
Copy link
Contributor

@begmaroman begmaroman commented Oct 2, 2023 •
edited

Description

No needed to check an account balance for eth_call. Accounts with zero balance still should be able to execute smart contract functions.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

@begmaroman begmaroman self-assigned this Oct 2, 2023
@begmaroman begmaroman added the bug fix Functionality that fixes a bug label Oct 2, 2023
@begmaroman begmaroman linked an issue Oct 2, 2023 that may be closed by this pull request
Price limit error “not enough funds to cover gas costs” for getter calls #1927
Closed
@begmaroman begmaroman marked this pull request as ready for review October 3, 2023 05:47
@begmaroman begmaroman changed the title Do not check account balance for eth_call Skip account balance check for eth_call Oct 3, 2023
Stefan-Ethernal
Stefan-Ethernal reviewed Oct 3, 2023
View reviewed changes
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Btw what should happen in the TraceCall, TraceTxn, and TraceBlock functions in the server.go? Should those executions be payable, since those are related to debug_* JSON RPC endpoints. I'm afraid that currently, they are payable, but it probably should not be from a logical standpoint. WDYT?

state/runtime/runtime.go Outdated Show resolved Hide resolved
state/executor.go Outdated Show resolved Hide resolved
@begmaroman begmaroman mentioned this pull request Oct 3, 2023
Closed
@begmaroman
Copy link
Contributor Author

@Stefan-Ethernal re debug endpoints. The logic inside those endpoints implies using an already existing state stored within a block. If a transaction was mined into the block, it means there were no issues with the balance. In this case, it just does not make sense to switch on this flag since a tx sender had enough balance within a specific block so the balance check is going to pass when tracing.

vcastellm
vcastellm reviewed Oct 3, 2023
View reviewed changes
state/executor.go Outdated Show resolved Hide resolved
vcastellm
vcastellm approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

Stefan-Ethernal
Stefan-Ethernal approved these changes Oct 3, 2023
View reviewed changes
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment •
edited

Choose a reason for hiding this comment

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

LGTM.

Btw would be awesome if we could include some e2e test (e.g. have dummy contract, account with 0 balance and make sure it is able to query the state of SC). We can do it through a separate PR if that makes sense to you.

@begmaroman
Copy link
Contributor Author

@Stefan-Ethernal added e2e test

igorcrevar
igorcrevar approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@igorcrevar igorcrevar left a comment

Choose a reason for hiding this comment

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

Can we have estimateGas and maybe even debugTransaction calls inside e2e test?

Approved

@begmaroman
Copy link
Contributor Author

@igorcrevar added e2e tests for those two endpoints.

@begmaroman begmaroman merged commit b455131 into develop Oct 4, 2023
6 checks passed
@begmaroman begmaroman deleted the feature/EVM-845 branch October 4, 2023 06:58
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vcastellm vcastellm vcastellm approved these changes

@igorcrevar igorcrevar igorcrevar approved these changes

@Stefan-Ethernal Stefan-Ethernal Stefan-Ethernal approved these changes

@dusan-maksimovic dusan-maksimovic dusan-maksimovic approved these changes

@stana-miric stana-miric stana-miric approved these changes

@goran-ethernal goran-ethernal Awaiting requested review from goran-ethernal

Assignees

@begmaroman begmaroman

Labels
bug fix Functionality that fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Price limit error “not enough funds to cover gas costs” for getter calls
6 participants
@begmaroman @vcastellm @igorcrevar @Stefan-Ethernal @dusan-maksimovic @stana-miric