Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix(rpc): align query result of future block for eth_getTransactionCount #1638

Merged
merged 12 commits into from
Jan 27, 2023

Conversation

mmsqe
Copy link
Contributor

@mmsqe mmsqe commented Jan 26, 2023

Closes: #XXX

Description

Get a future block number 20000000 (0x1312d00) and a address that has a transaction count of 0:

curl --location --request POST 'https://evm-t3.cronos.org' \
--header 'Content-Type: application/json' \
--data-raw '{
    "jsonrpc": "2.0",
    "method": "eth_getTransactionCount",
    "params": [
        "0xdEADBEeF00000000000000000000000000000000",
        "0x1312d00"
    ],
    "id": 0
}'

Current Result: {"id":0,"result":"0x0","jsonrpc":"2.0"}

Align with result when get a future block number 20000000 (0x1312d00) and an address with a non-zero transaction count:

curl --location --request POST 'https://evm-t3.cronos.org' \
--header 'Content-Type: application/json' \
--data-raw '{
    "jsonrpc": "2.0",
    "method": "eth_getTransactionCount",
    "params": [
        "0xc05A754131288197F958CF36C6C807FAF5Ec12D6",
        "0x1312d00"
    ],
    "id": 0
}'

Result: {"id":0,"error":{"code":-32000,"message":"rpc error: code = Unknown desc = codespace sdk code 26: invalid height: cannot query with height in the future; please provide a valid height","data":null},"jsonrpc":"2.0"}


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@mmsqe mmsqe marked this pull request as ready for review January 26, 2023 03:13
@mmsqe mmsqe requested a review from a team as a code owner January 26, 2023 03:13
@mmsqe mmsqe requested review from Vvaradinov and danburck and removed request for a team January 26, 2023 03:13
return &n, err
}
height := blockNum.Int64()
currentHeight := int64(bn)

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion

Potential integer overflow by integer type conversion
@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #1638 (ef5bdaf) into main (7213718) will increase coverage by 0.00%.
The diff coverage is 75.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1638   +/-   ##
=======================================
  Coverage   68.19%   68.20%           
=======================================
  Files         112      112           
  Lines       10155    10167   +12     
=======================================
+ Hits         6925     6934    +9     
- Misses       2825     2827    +2     
- Partials      405      406    +1     
Impacted Files Coverage Δ
rpc/backend/account_info.go 82.66% <75.00%> (-0.67%) ⬇️

Copy link
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

tACK! Thanks for aligning the behaviours @mmsqe! Left two small suggestions for more verbosity

CHANGELOG.md Outdated Show resolved Hide resolved
rpc/backend/account_info.go Outdated Show resolved Hide resolved
mmsqe and others added 2 commits January 27, 2023 19:33
Co-authored-by: MalteHerrmann <[email protected]>
return &n, err
}
height := blockNum.Int64()
currentHeight := int64(bn)

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion

Potential integer overflow by integer type conversion
@MalteHerrmann MalteHerrmann merged commit 1a2ee06 into evmos:main Jan 27, 2023
@mmsqe mmsqe deleted the query_future_acc branch March 5, 2023 02:43
mmsqe added a commit to mmsqe/ethermint that referenced this pull request Mar 14, 2023
…unt (evmos#1638)

* add future height check for get_transaction_count

* add query future account test

* fasten get_transaction_count test

* add change doc

* fix test

* update nix

* Update CHANGELOG.md

Co-authored-by: MalteHerrmann <[email protected]>

* Update rpc/backend/account_info.go

Co-authored-by: MalteHerrmann <[email protected]>

* update nix

* add test for block height in future

Co-authored-by: MalteHerrmann <[email protected]>
Co-authored-by: MalteHerrmann <[email protected]>
mmsqe added a commit to crypto-org-chain/ethermint that referenced this pull request Mar 14, 2023
…unt (evmos#1638) (#225)

* add future height check for get_transaction_count

* add query future account test

* fasten get_transaction_count test

* add change doc

* fix test

* update nix

* Update CHANGELOG.md



* Update rpc/backend/account_info.go



* update nix

* add test for block height in future

Co-authored-by: MalteHerrmann <[email protected]>
Co-authored-by: MalteHerrmann <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants