Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement validateGasWanted() #48

Merged
merged 4 commits into from
Jan 6, 2021
Merged

feat: implement validateGasWanted() #48

merged 4 commits into from
Jan 6, 2021

Conversation

jinsan-line
Copy link
Contributor

@jinsan-line jinsan-line commented Jan 5, 2021

Related with: https://github.com/line/link/issues/1153, tendermint/tendermint#5675, tendermint/tendermint#3546, Finschia/ostracon#158, (Finschia/ostracon#157)

Description

Validate GasWanted during ante handler. It's need to remove tendermint mempool.postCheck().
It should reconstruct the same logic with https://github.com/line/tendermint/blob/a01efd28c7257d3b056d1fcf263d39ccea3c1273/mempool/mempool.go#L126

Motivation and context

How has this been tested?

make test

Checklist:

  • I followed the contributing guidelines.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@jinsan-line jinsan-line self-assigned this Jan 5, 2021
@jinsan-line jinsan-line marked this pull request as draft January 5, 2021 07:34
@@ -41,6 +41,11 @@ func (sud SetUpContextDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate

newCtx = SetGasMeter(simulate, ctx, gasTx.GetGas())

err = validateGasWanted(newCtx)
if err != nil {
return newCtx, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, err.Error())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is ErrOutOfGas proper for this error? I'd like to hear your opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it is proper error

@jinsan-line jinsan-line marked this pull request as ready for review January 5, 2021 08:54
Copy link

@egonspace egonspace left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kukugi kukugi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -41,6 +41,11 @@ func (sud SetUpContextDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate

newCtx = SetGasMeter(simulate, ctx, gasTx.GetGas())

err = validateGasWanted(newCtx)
if err != nil {
return newCtx, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it is proper error

@jinsan-line jinsan-line merged commit 900473c into Finschia:feat/perf Jan 6, 2021
@jinsan-line jinsan-line deleted the check-gas-in-ante branch January 6, 2021 07:15
@wetcod wetcod mentioned this pull request Feb 4, 2021
3 tasks
wetcod added a commit that referenced this pull request Feb 5, 2021
* build: bump up tendermint to v0.33.9-0.1.0-rc2

* feat: implement `validateGasWanted()` (#48)

* feat: impl `validateGasWanted()`

* fix: lint error

* chore: bump up tendermint

* fix: tests to use `grpc` instead of `socket`

* fix: revert the transport of the server to socket

Co-authored-by: KIm, JinSan <jinsan@linecorp.com>
jinsan-line added a commit that referenced this pull request Apr 22, 2021
* feat: impl `validateGasWanted()`

* fix: lint error

* chore: bump up tendermint

* fix: tests to use `grpc` instead of `socket`
# Conflicts:
#	go.mod
#	go.sum
#	server/start.go
#	x/genutil/client/cli/init_test.go
jinsan-line added a commit that referenced this pull request Apr 22, 2021
* feat: impl `validateGasWanted()`

* fix: lint error

* chore: bump up tendermint

* fix: tests to use `grpc` instead of `socket`
# Conflicts:
#	go.mod
#	go.sum
#	server/start.go
#	x/genutil/client/cli/init_test.go
@0Tech 0Tech mentioned this pull request Sep 14, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants