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

Fix grpc query failure on legacy blocks#1354

Merged
fedekunze merged 4 commits intoevmos:mainfrom
yihuang:main
Sep 23, 2022
Merged

Fix grpc query failure on legacy blocks#1354
fedekunze merged 4 commits intoevmos:mainfrom
yihuang:main

Conversation

@yihuang
Copy link

@yihuang yihuang commented Sep 21, 2022 •
edited
Loading

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)

@yihuang yihuang requested a review from a team as a code owner September 21, 2022 04:59
@yihuang yihuang requested review from austinchandra and ramacarlucho and removed request for a team September 21, 2022 04:59
@yihuang yihuang mentioned this pull request Sep 21, 2022
Closed
@codecov
Copy link

codecov bot commented Sep 21, 2022 •
edited
Loading

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%) ⬆️

fedekunze
fedekunze reviewed Sep 21, 2022
View reviewed changes
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
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.

yihuang
yihuang commented Sep 22, 2022
View reviewed changes
Problem: grpc query fail on legacy blocks
bec1a6a 
`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 yihuang mentioned this pull request Sep 22, 2022
Merged
13 tasks
@yihuang
Copy link
Author

yihuang commented Sep 22, 2022 •
edited
Loading

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
Author

yihuang commented Sep 22, 2022 •
edited
Loading

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 austinchandra and ramacarlucho September 22, 2022 16:07
yihuang
yihuang commented Sep 23, 2022
View reviewed changes
@fedekunze
Copy link
Contributor

fedekunze commented Sep 23, 2022

@yihuang some unit tests are failing

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

yihuang commented Sep 23, 2022 •
edited
Loading

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

fedekunze 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

@yihuang
Copy link
Author

yihuang commented Sep 23, 2022 •
edited
Loading 

--- 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 disabled auto-merge September 23, 2022 16:06
@fedekunze fedekunze merged commit 35850e6 into evmos:main Sep 23, 2022
@VictorTrustyDev VictorTrustyDev mentioned this pull request Oct 20, 2022
Closed
@danburck danburck mentioned this pull request Nov 30, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ramacarlucho ramacarlucho Awaiting requested review from ramacarlucho

@facs95 facs95 Awaiting requested review from facs95

1 more reviewer

@fedekunze fedekunze fedekunze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yihuang @fedekunze