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

update minimum gas price to be 1 #515

Merged
merged 2 commits into from
Sep 15, 2020
Merged

update minimum gas price to be 1 #515

merged 2 commits into from
Sep 15, 2020

Conversation

noot
Copy link
Contributor

@noot noot commented Sep 15, 2020

Closes: #XXX

Description


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)

@noot noot requested a review from fedekunze as a code owner September 15, 2020 16:55
@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #515 into development will decrease coverage by 0.10%.
The diff coverage is 33.33%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #515      +/-   ##
===============================================
- Coverage        72.14%   72.03%   -0.11%     
===============================================
  Files               41       41              
  Lines             2714     2718       +4     
===============================================
  Hits              1958     1958              
- Misses             613      615       +2     
- Partials           143      145       +2     
Impacted Files Coverage Δ
x/evm/types/msg.go 62.73% <33.33%> (-1.60%) ⬇️

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 cf9662c...a4b0942. Read the comment docs.

Copy link
Contributor

@araskachoi araskachoi left a comment

Choose a reason for hiding this comment

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

lg2m

@noot noot merged commit 9a2d39b into development Sep 15, 2020
@noot noot deleted the noot/gas-price branch September 15, 2020 17:17
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, @noot! One minor comment. What was the context for changing the minimum gas price to 1?

@@ -185,8 +189,12 @@ func (msg MsgEthereumTx) Type() string { return TypeMsgEthereumTx }
// ValidateBasic implements the sdk.Msg interface. It performs basic validation
// checks of a Transaction. If returns an error if validation fails.
func (msg MsgEthereumTx) ValidateBasic() error {
if msg.Data.Price.Cmp(big.NewInt(0)) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, we should add a unit test case

@@ -82,8 +82,12 @@ func (msg MsgEthermint) GetSignBytes() []byte {

// ValidateBasic runs stateless checks on the message
func (msg MsgEthermint) ValidateBasic() error {
if msg.Price.IsZero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@noot
Copy link
Contributor Author

noot commented Sep 22, 2020

@fedekunze the main reason for this PR was to prepare for a public testnet, since if txs with gasPrice = 0 are accepted, then transactions are free and the network can be spammed. will add some unit tests!

@noot noot mentioned this pull request Sep 23, 2020
11 tasks
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.

3 participants