Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

evm: params #458

Merged
merged 32 commits into from
Sep 2, 2020
Merged

evm: params #458

merged 32 commits into from
Sep 2, 2020

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Aug 18, 2020

Closes: #443

Description

TODO:

  • param tests

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 Aug 18, 2020

Codecov Report

Merging #458 into development will increase coverage by 1.29%.
The diff coverage is 94.41%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #458      +/-   ##
===============================================
+ Coverage        70.15%   71.45%   +1.29%     
===============================================
  Files               38       40       +2     
  Lines             2520     2662     +142     
===============================================
+ Hits              1768     1902     +134     
- Misses             620      624       +4     
- Partials           132      136       +4     
Impacted Files Coverage Δ
x/evm/types/key.go 50.00% <ø> (ø)
x/evm/handler.go 91.45% <50.00%> (-3.15%) ⬇️
x/evm/keeper/querier.go 71.22% <50.00%> (ø)
x/evm/keeper/keeper.go 73.43% <86.66%> (+3.43%) ⬆️
app/ante/eth.go 65.35% <89.47%> (+1.17%) ⬆️
x/evm/types/chain_config.go 97.59% <97.50%> (-2.41%) ⬇️
app/ante/ante.go 63.33% <100.00%> (ø)
app/ethermint.go 88.23% <100.00%> (+0.06%) ⬆️
x/evm/genesis.go 76.74% <100.00%> (+3.05%) ⬆️
x/evm/keeper/params.go 100.00% <100.00%> (ø)
... and 8 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 26816e2...1b8a25b. Read the comment docs.

@fedekunze fedekunze added the x/evm EVM module issues label Aug 18, 2020
@fedekunze fedekunze marked this pull request as ready for review September 1, 2020 16:34
@fedekunze fedekunze requested a review from noot as a code owner September 1, 2020 16:34
* attempt to fix

* cleanup

* add idx check

* update csdb.Copy
@fedekunze fedekunze added this to the v0.2.0 milestone Sep 1, 2020
ConstantinopleBlock: big.NewInt(0),
PetersburgBlock: big.NewInt(0),
EIP150Block: sdk.ZeroInt(),
EIP150Hash: "0x2086799aeebeae135c246c65021c82b4e15a2c451340993aacfd2751886514f0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think this default hash is invalid should be populated on initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noot @araskachoi thoughts?

@fedekunze
Copy link
Contributor Author

the simulations are failing because the evm module is not supported yet on sims :

--- FAIL: TestAppSimulationAfterImport (101.00s)
panic: UnmarshalJSON cannot decode empty bytes [recovered]
	panic: UnmarshalJSON cannot decode empty bytes

goroutine 61 [running]:
testing.tRunner.func1.1(0x4d37e00, 0xc004197540)
	/usr/local/Cellar/go/1.15/libexec/src/testing/testing.go:1057 +0x30d
testing.tRunner.func1(0xc000602c00)
	/usr/local/Cellar/go/1.15/libexec/src/testing/testing.go:1060 +0x41a
panic(0x4d37e00, 0xc004197540)
	/usr/local/Cellar/go/1.15/libexec/src/runtime/panic.go:969 +0x175
github.com/cosmos/cosmos-sdk/x/params/subspace.Subspace.Get(0xc000266000, 0x51ae9e0, 0xc000023e50, 0x51aea20, 0xc000023ea0, 0xc000038190, 0x3, 0xb, 0xc0002038f0, 0x51bea60, ...)
	/Users/federico/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.39.1/x/params/subspace/subspace.go:106 +0x2ca
github.com/cosmos/cosmos-sdk/x/params/subspace.Subspace.GetParamSet(0xc000266000, 0x51ae9e0, 0xc000023e50, 0x51aea20, 0xc000023ea0, 0xc000038190, 0x3, 0xb, 0xc0002038f0, 0x51bea60, ...)
	/Users/federico/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.39.1/x/params/subspace/subspace.go:217 +0x1a8
github.com/cosmos/ethermint/x/evm/types.(*CommitStateDB).GetParams(...)
	/Users/federico/ethermint/x/evm/types/statedb.go:250
github.com/cosmos/ethermint/x/evm/keeper.Keeper.GetParams(0xc000266000, 0x51ae9e0, 0xc000023e80, 0xc00014f800, 0x0, 0xc0002ccd40, 0x51bea60, 0xc000126008, 0x51cc000, 0xc0021db280, ...)
	/Users/federico/ethermint/x/evm/keeper/params.go:11 +0x1b4
github.com/cosmos/ethermint/x/evm.ExportGenesis(0x51bea60, 0xc000126008, 0x51cc000, 0xc0021db280, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/federico/ethermint/x/evm/genesis.go:86 +0x279
github.com/cosmos/ethermint/x/evm.AppModule.ExportGenesis(0xc000266000, 0x51ae9e0, 0xc000023e80, 0xc00014f800, 0x0, 0xc0002ccd40, 0x51c20e0, 0xc000266a80, 0x51bea60, 0xc000126008, ...)
	/Users/federico/ethermint/x/evm/module.go:133 +0xa8
github.com/cosmos/cosmos-sdk/types/module.(*Manager).ExportGenesis(0xc000266b60, 0x51bea60, 0xc000126008, 0x51cc000, 0xc0021db280, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/federico/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.39.1/types/module/module.go:285 +0xfe
github.com/cosmos/ethermint/app.(*EthermintApp).ExportAppStateAndValidators(0xc000180b00, 0xc000122001, 0xc003119c70, 0x0, 0x0, 0x0, 0x0, 0x15, 0x0, 0x0, ...)
	/Users/federico/ethermint/app/export.go:40 +0x125
github.com/cosmos/ethermint/app.TestAppSimulationAfterImport(0xc000602c00)
	/Users/federico/ethermint/app/simulation_test.go:212 +0x71a
testing.tRunner(0xc000602c00, 0x50574d0)
	/usr/local/Cellar/go/1.15/libexec/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
	/usr/local/Cellar/go/1.15/libexec/src/testing/testing.go:1159 +0x386
FAIL	github.com/cosmos/ethermint/app	101.773s

@fedekunze
Copy link
Contributor Author

Not sure what's going on with the storage rpc test tho:

=== RUN   TestEth_ExportAccount_WithStorage
    rpc_test.go:837: 
        	Error Trace:	rpc_test.go:837
        	Error:      	Not equal: 
        	            	expected: hexutil.Bytes{0x60, 0x80, 0x60, 0x40, 0x52, 0x34, 0x80, 0x15, 0x60, 0xf, 0x57, 0x60, 0x0, 0x80, 0xfd, 0x5b, 0x50, 0x60, 0x4, 0x36, 0x10, 0x60, 0x28, 0x57, 0x60, 0x0, 0x35, 0x60, 0xe0, 0x1c, 0x80, 0x63, 0xeb, 0x8a, 0xc9, 0x21, 0x14, 0x60, 0x2d, 0x57, 0x5b, 0x60, 0x0, 0x80, 0xfd, 0x5b, 0x60, 0x60, 0x60, 0x4, 0x80, 0x36, 0x3, 0x60, 0x40, 0x81, 0x10, 0x15, 0x60, 0x41, 0x57, 0x60, 0x0, 0x80, 0xfd, 0x5b, 0x81, 0x1, 0x90, 0x80, 0x80, 0x35, 0x90, 0x60, 0x20, 0x1, 0x90, 0x92, 0x91, 0x90, 0x80, 0x35, 0x90, 0x60, 0x20, 0x1, 0x90, 0x92, 0x91, 0x90, 0x50, 0x50, 0x50, 0x60, 0x62, 0x56, 0x5b, 0x0, 0x5b, 0x81, 0x60, 0x0, 0x81, 0x90, 0x55, 0x50, 0x80, 0x82, 0x7f, 0xf3, 0xca, 0x12, 0x4a, 0x69, 0x7b, 0xa0, 0x7e, 0x8c, 0x5e, 0x80, 0xbe, 0xbc, 0xfc, 0xc4, 0x89, 0x91, 0xfc, 0x16, 0xa6, 0x31, 0x70, 0xe8, 0xa9, 0x20, 0x6e, 0x30, 0x50, 0x89, 0x60, 0xd0, 0x3, 0x60, 0x40, 0x51, 0x60, 0x40, 0x51, 0x80, 0x91, 0x3, 0x90, 0xa3, 0x50, 0x50, 0x56, 0xfe, 0xa2, 0x65, 0x62, 0x7a, 0x7a, 0x72, 0x31, 0x58, 0x20, 0x1d, 0x94, 0xd2, 0x18, 0x7a, 0xaf, 0x3a, 0x67, 0x90, 0x52, 0x7b, 0x61, 0x5f, 0xcc, 0x40, 0x97, 0xf, 0xeb, 0xf0, 0x38, 0x5f, 0xa6, 0xd7, 0x2a, 0x23, 0x44, 0x84, 0x8e, 0xbd, 0xd, 0xf3, 0xe9, 0x64, 0x73, 0x6f, 0x6c, 0x63, 0x43, 0x0, 0x5, 0x11, 0x0, 0x32}
        	            	actual  : hexutil.Bytes(nil)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,16 +1,2 @@
        	            	-(hexutil.Bytes) (len=208) {
        	            	- 00000000  60 80 60 40 52 34 80 15  60 0f 57 60 00 80 fd 5b  |`.`@R4..`.W`...[|
        	            	- 00000010  50 60 04 36 10 60 28 57  60 00 35 60 e0 1c 80 63  |P`.6.`(W`.5`...c|
        	            	- 00000020  eb 8a c9 21 14 60 2d 57  5b 60 00 80 fd 5b 60 60  |...!.`-W[`...[``|
        	            	- 00000030  60 04 80 36 03 60 40 81  10 15 60 41 57 60 00 80  |`..6.`@...`AW`..|
        	            	- 00000040  fd 5b 81 01 90 80 80 35  90 60 20 01 90 92 91 90  |.[.....5.` .....|
        	            	- 00000050  80 35 90 60 20 01 90 92  91 90 50 50 50 60 62 56  |.5.` .....PPP`bV|

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

the solidity tests fail on this branch with Error: while migrating Migrations: The contract code couldn't be stored, please check your gas limit.

same with deploying aragonOS. this is probably why the export account test is failing

I can't tell exactly why from looking at the changes, going to look into it more

@noot noot mentioned this pull request Sep 2, 2020
11 tasks
@fedekunze
Copy link
Contributor Author

the solidity tests fail on this branch with Error: while migrating Migrations: The contract code couldn't be stored, please check your gas limit.
same with deploying aragonOS. this is probably why the export account test is failing

The ChainConfig has to do with the gas cost updates. Prob looking into that is the first thing we should do

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

contract deployments work for me now, looks good!

err := json.Unmarshal(rpcRes.Result, &gas)
require.NoError(t, err, string(rpcRes.Result))

require.Equal(t, "0xffdf", gas.String())
require.Equal(t, "0x1051d", gas)
Copy link
Contributor Author

@fedekunze fedekunze Sep 2, 2020

Choose a reason for hiding this comment

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

not sure if we should address this? technically it should remain the same? @noot

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it should likely be the same, do we have any tests for actual gas consumption vs estimation? if not we should add some

@fedekunze fedekunze merged commit 792c1ff into development Sep 2, 2020
@fedekunze fedekunze deleted the evm-params branch September 2, 2020 19:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Genesis Parameters
2 participants