Problem: ethermint v0.9.0 is not used #241

yihuang · 2021-12-03T10:06:04Z

Closes: #238

Solution:

update ethermint dependency.
use a constant mock feemarket keeper for now.

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

Thank you for your code, it's appreciated! :)

Closes: crypto-org-chain#238 Solution: - update ethermint dependency. - use a constant mock feemarket keeper for now.

go.mod

app/feemarketkeeper.go

thomas-nguy · 2021-12-06T01:35:47Z

I think we will need to increment the version for the upgrade handler as well

yihuang · 2021-12-06T04:10:14Z

I think we will need to increment the version for the upgrade handler as well

Added an upgrade handler for the next major version number v0.7.0.
I think the ConsensusVersion number of cronos module don't need to update because there's nothing to migrate in that module?

codecov · 2021-12-06T08:53:16Z

Codecov Report

Merging #241 (49eb101) into main (3ea70c5) will increase coverage by 3.29%.
The diff coverage is 0.33%.

@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   21.51%   24.80%   +3.29%     
==========================================
  Files          27       34       +7     
  Lines        1729     2572     +843     
==========================================
+ Hits          372      638     +266     
- Misses       1324     1887     +563     
- Partials       33       47      +14

Impacted Files	Coverage Δ
app/prefix.go	`0.00% <0.00%> (ø)`
app/test_helpers.go	`0.00% <0.00%> (ø)`
x/cronos/genesis.go	`61.53% <ø> (-38.47%)`	⬇️
x/cronos/handler.go	`100.00% <ø> (ø)`
x/cronos/keeper/evm.go	`56.31% <ø> (+4.58%)`	⬆️
x/cronos/keeper/evm_hooks.go	`90.00% <ø> (ø)`
x/cronos/keeper/evm_log_handlers.go	`83.50% <ø> (ø)`
x/cronos/keeper/gravity_hooks.go	`0.00% <ø> (ø)`
x/cronos/keeper/grpc_query.go	`0.00% <ø> (ø)`
x/cronos/keeper/ibc.go	`83.20% <ø> (+5.01%)`	⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a4283...49eb101. Read the comment docs.

go.mod

cmd/cronosd/config/prefix_mainnet.go

yihuang · 2021-12-07T04:51:11Z

integration tests passed.

thomas-nguy · 2021-12-07T07:10:38Z

is it still a draft?

yihuang · 2021-12-07T08:25:32Z

is it still a draft?

changed, it's ready for review and merge.

yihuang · 2021-12-07T08:27:13Z

go.mod

Suggested change

replace github.com/tharsis/ethermint => github.com/crypto-org-chain/ethermint v0.6.1-0.20211207040748-254df3803d62

// https://github.com/crypto-org-chain/ethermint/tree/v0.9.x-cronos

replace github.com/tharsis/ethermint => github.com/crypto-org-chain/ethermint v0.6.1-0.20211207040748-254df3803d62

go.mod

thomas-nguy · 2021-12-07T09:04:55Z

lgtm

@tomtau could you review the upgrade section part? I think no there is no migration to be done on ethermint side

tomtau · 2021-12-07T09:35:22Z

app/app.go

+	// upgrade handler
+	plan0_7_0 := "v0.7.0"
+	app.UpgradeKeeper.SetUpgradeHandler(plan0_7_0, func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
+		return app.mm.RunMigrations(ctx, app.configurator, fromVM)


not sure if the ibc-go module version increased? https://github.com/cosmos/ibc-go/blob/main/modules/core/module.go#L164
there isn't much change between ibc-go 1 and 2, but not sure whether one shouldn't explicitly pass it in the upgrade handler so that the version map gets upgraded for the future

I think the fromVM is loaded from the storage, and RunMigrations will migrate them to current version automatically, so it should work?

if that's desirable with the fork to upstream migration later (I'm assuming when it adds the hooks or middleware, it'll be increased to 3?)

if that's desirable with the fork to upstream migration later (I'm assuming when it adds the hooks or middleware, it'll be increased to 3?)

do you mean the cronos module? I think if there's nothing to migrate in the storage, there's no need to increase the consensus version, right?

I meant the ibc-go fork. ConsensusVersion in a module should be increased with any state-breaking changes, so hopefully it's fine

tomtau

I think fee market has some on-chain parameters, so maybe those should be initialised in the upgrade handler?

yihuang · 2021-12-07T09:38:43Z

I think fee market has some on-chain parameters, so maybe those should be initialised in the upgrade handler?

RunMigrations will run InitGenesis if the module is not in fromVM but in current app, I think that should do it?

tomtau · 2021-12-07T09:54:47Z

I think fee market has some on-chain parameters, so maybe those should be initialised in the upgrade handler?

RunMigrations will run InitGenesis if the module is not in fromVM but in current app, I think that should do it?

but fromVM will contain the old store's version map, not the one being migrated to?

yihuang · 2021-12-07T11:01:55Z

I think fee market has some on-chain parameters, so maybe those should be initialised in the upgrade handler?

RunMigrations will run InitGenesis if the module is not in fromVM but in current app, I think that should do it?

but fromVM will contain the old store's version map, not the one being migrated to?

fromVM is the version before upgrade, it'll compare it with current one and execute the migration, feemarket is not contained in fromVM but exists in current app, so it'll call InitGenesis and store the new version number, https://github.com/cosmos/cosmos-sdk/blob/master/types/module/module.go#L432.
That's my understanding from the code, haven't tested it yet, opened an issue to test it in integration test: #247

tomtau · 2021-12-08T01:11:05Z

ok, and is feemarket's DefaultGenesis genesis reasonable or would that break or make things crazy (e.g. unreasonably low or high costs)?

yihuang · 2021-12-08T01:32:05Z

ok, and is feemarket's DefaultGenesis genesis reasonable or would that break or make things crazy (e.g. unreasonably low or high costs)?

Hmm, that's sth need to be aware of, the default InitialBaseFee(1000000000) happens to be much lower than our validator minimal gas price (5000000000000), both constraints are enforced, so it's ok.
I'll add a comment about this.

app/app.go

tomtau · 2021-12-08T02:39:56Z

go.mod


-replace github.com/cosmos/iavl => github.com/cosmos/iavl v0.17.1
+replace github.com/cosmos/iavl => github.com/cosmos/iavl v0.17.2


v0.17.2 had some race condition / concurrent access issues -- not sure if this can manifest in Ethermint / Web3 APIs: cosmos/iavl@v0.17.2...v0.17.3 but perhaps better to use the latest one?

tomtau

the mainnet beta upgrade should be better planned, so for the feemarket network param value, the upgrade handler can be later adjusted

thomas-nguy · 2021-12-08T02:43:03Z

can you add an entry on the changelog?

yihuang · 2021-12-08T02:48:29Z

can you add an entry on the changelog?

done

Problem: ethermint v0.9.0 is not used

b3222e7

Closes: crypto-org-chain#238 Solution: - update ethermint dependency. - use a constant mock feemarket keeper for now.

yihuang commented Dec 3, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

yihuang commented Dec 3, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

thomas-nguy reviewed Dec 6, 2021

View reviewed changes

app/feemarketkeeper.go Outdated Show resolved Hide resolved

yihuang added 4 commits December 6, 2021 10:32

ibc-go repo

32a83ad

update gomod2nix

c781c88

add feemarket module

34dd05b

add upgrade handler to add feemarket module

6944c31

yihuang added 4 commits December 6, 2021 13:34

fix linter

4cea377

fix unit tests

9646a8a

update gravity-bridge to ibc-go 2.0

4011dc9

fix integration tests

9694d47

disable feemarket

90e6ffb

thomas-nguy reviewed Dec 7, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

update gravity-bridge dependency

a6dbe3f

thomas-nguy reviewed Dec 7, 2021

View reviewed changes

cmd/cronosd/config/prefix_mainnet.go Show resolved Hide resolved

fix exported genesis test

3cfefc7

use crypto-org-chain

9b5a07e

yihuang marked this pull request as ready for review December 7, 2021 08:25

yihuang requested a review from a team as a code owner December 7, 2021 08:25

yihuang requested review from JayT106 and devashishdxt and removed request for a team December 7, 2021 08:25

yihuang commented Dec 7, 2021

View reviewed changes

thomas-nguy reviewed Dec 7, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

thomas-nguy approved these changes Dec 7, 2021

View reviewed changes

tomtau reviewed Dec 7, 2021

View reviewed changes

yihuang commented Dec 8, 2021

View reviewed changes

app/app.go Show resolved Hide resolved

add comment about default feemarket parameters

60d3e63

tomtau reviewed Dec 8, 2021

View reviewed changes

tomtau approved these changes Dec 8, 2021

View reviewed changes

yihuang added 2 commits December 8, 2021 10:43

fix linter

26ff78c

changelog

766847a

yihuang added 2 commits December 8, 2021 10:52

upgrade minor version

1ab9582

comment ethermint fork branch name

49eb101

yihuang merged commit e4f0455 into crypto-org-chain:main Dec 8, 2021

yihuang deleted the ethermint-mai branch December 8, 2021 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: ethermint v0.9.0 is not used #241

Problem: ethermint v0.9.0 is not used #241

yihuang commented Dec 3, 2021

thomas-nguy commented Dec 6, 2021 •

edited

Loading

yihuang commented Dec 6, 2021

codecov bot commented Dec 6, 2021 •

edited

Loading

yihuang commented Dec 7, 2021

thomas-nguy commented Dec 7, 2021

yihuang commented Dec 7, 2021

yihuang Dec 7, 2021

thomas-nguy commented Dec 7, 2021

tomtau Dec 7, 2021

yihuang Dec 7, 2021

tomtau Dec 8, 2021

yihuang Dec 8, 2021

tomtau Dec 8, 2021

tomtau left a comment

yihuang commented Dec 7, 2021

tomtau commented Dec 7, 2021

yihuang commented Dec 7, 2021

tomtau commented Dec 8, 2021

yihuang commented Dec 8, 2021

tomtau Dec 8, 2021

yihuang Dec 8, 2021

tomtau left a comment

thomas-nguy commented Dec 8, 2021

yihuang commented Dec 8, 2021

	replace github.com/tharsis/ethermint => github.com/crypto-org-chain/ethermint v0.6.1-0.20211207040748-254df3803d62
	// https://github.com/crypto-org-chain/ethermint/tree/v0.9.x-cronos
	replace github.com/tharsis/ethermint => github.com/crypto-org-chain/ethermint v0.6.1-0.20211207040748-254df3803d62


		replace github.com/cosmos/iavl => github.com/cosmos/iavl v0.17.1
		replace github.com/cosmos/iavl => github.com/cosmos/iavl v0.17.2

Problem: ethermint v0.9.0 is not used #241

Problem: ethermint v0.9.0 is not used #241

Conversation

yihuang commented Dec 3, 2021

PR Checklist:

thomas-nguy commented Dec 6, 2021 • edited Loading

yihuang commented Dec 6, 2021

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

yihuang commented Dec 7, 2021

thomas-nguy commented Dec 7, 2021

yihuang commented Dec 7, 2021

Choose a reason for hiding this comment

thomas-nguy commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

yihuang commented Dec 7, 2021

tomtau commented Dec 7, 2021

yihuang commented Dec 7, 2021

tomtau commented Dec 8, 2021

yihuang commented Dec 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

thomas-nguy commented Dec 8, 2021

yihuang commented Dec 8, 2021

thomas-nguy commented Dec 6, 2021 •

edited

Loading

codecov bot commented Dec 6, 2021 •

edited

Loading