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

Fix grpc query failure on legacy blocks #1354

Merged
merged 4 commits into from
Sep 23, 2022
Merged

Fix grpc query failure on legacy blocks #1354

merged 4 commits into from
Sep 23, 2022

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Sep 21, 2022

Description

Mainly BaseFee and EthCall.
Solution:

  • since the grpc query handlers are used for all versions of the blocks, it needs to be compatible with legacy formats.

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)

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #1354 (334365c) into main (6574019) will increase coverage by 0.01%.
The diff coverage is 65.21%.

❗ Current head 334365c differs from pull request most recent head 3796527. Consider uploading reports for the commit 3796527 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1354      +/-   ##
==========================================
+ Coverage   54.77%   54.78%   +0.01%     
==========================================
  Files         107      108       +1     
  Lines        9994    10015      +21     
==========================================
+ Hits         5474     5487      +13     
- Misses       4249     4255       +6     
- Partials      271      273       +2     
Impacted Files Coverage Δ
x/evm/keeper/keeper.go 78.36% <0.00%> (-1.88%) ⬇️
app/upgrades.go 40.00% <40.00%> (ø)
x/feemarket/keeper/keeper.go 91.30% <85.71%> (-1.01%) ⬇️
app/app.go 86.07% <100.00%> (ø)
x/feemarket/keeper/params.go 86.36% <100.00%> (+4.01%) ⬆️

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

thanks @yihuang. Can you add unit and integration tests for this change?

@yihuang
Copy link
Contributor Author

yihuang commented Sep 21, 2022

thanks @yihuang. Can you add unit and integration tests for this change?

sure, I'm still investigating another case with eth_call, probably similar root cause.

CHANGELOG.md Outdated Show resolved Hide resolved
`BaseFee` and `EthCall`.

Solution:
- since grpc query handlers are used for all versions of the blocks, it need to be compatible with legacy formats.

debug

fix basefee fetch

Revert "debug"

This reverts commit 50ebaf697fc06b0d6e26abd8de8f89717e8a219d.

update gomod2nix

Update CHANGELOG.md

debug

fix panic

Revert "debug"

This reverts commit e08af04b0776bd390c42706cc9ec978e00bcb3bb.
@yihuang yihuang changed the title Problem: grpc query fail to query v1 version of base fee Fix grpc query failure on legacy blocks Sep 22, 2022
@yihuang
Copy link
Contributor Author

yihuang commented Sep 22, 2022

thanks @yihuang. Can you add unit and integration tests for this change?

seems hard to do a unit test for this, I'll port the integration test in cronos to here.

@yihuang
Copy link
Contributor Author

yihuang commented Sep 22, 2022

thanks @yihuang. Can you add unit and integration tests for this change?

integration test added, and we also need to update cosmos-sdk to recent 0.46.x to make it pass.

@yihuang yihuang requested review from fedekunze and removed request for 0a1c and ramacarlucho September 22, 2022 16:07
@fedekunze
Copy link
Contributor

@yihuang some unit tests are failing

@fedekunze fedekunze enabled auto-merge (squash) September 23, 2022 06:25
@yihuang
Copy link
Contributor Author

yihuang commented Sep 23, 2022

interesting, it doesn't reproduce locally, can you try rerun?
there's a suspicious msg in the CI log: fatal: No names found, cannot describe anything., don't appear in local run, not sure what that means.

@fedekunze
Copy link
Contributor

--- FAIL: TestKeyring (0.29s)
    algorithm_test.go:78: 
        	Error Trace:	/home/runner/work/ethermint/ethermint/crypto/hd/algorithm_test.go:78
        	Error:      	Not equal: 
        	            	expected: "0x25635F7750082d85CcA2D97d1e6086490cBeD0D0"
        	            	actual  : "0x1738E2725c0122705265421F8173AfA75c004CB6"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-0x25635F7750082d85CcA2D97d1e6086490cBeD0D0
        	            	+0x1738E2725c0122705265421F8173AfA75c004CB6
        	Test:       	TestKeyring

@yihuang
Copy link
Contributor Author

yihuang commented Sep 23, 2022

--- FAIL: TestKeyring (0.29s)
    algorithm_test.go:78: 
        	Error Trace:	/home/runner/work/ethermint/ethermint/crypto/hd/algorithm_test.go:78
        	Error:      	Not equal: 
        	            	expected: "0x25635F7750082d85CcA2D97d1e6086490cBeD0D0"
        	            	actual  : "0x1738E2725c0122705265421F8173AfA75c004CB6"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-0x25635F7750082d85CcA2D97d1e6086490cBeD0D0
        	            	+0x1738E2725c0122705265421F8173AfA75c004CB6
        	Test:       	TestKeyring

It don't reproduce when run locally, and didn't seem to be related to this PR, maybe has sth to do with SDK update.

@fedekunze fedekunze merged commit 35850e6 into evmos:main Sep 23, 2022
@danburck danburck mentioned this pull request Nov 30, 2022
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