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

improve error message in SendTransaction json-rpc api #786

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Nov 26, 2021

Description

Closes: #785

Solution:

  • Remove the stacktrace dependency, use sdkerrors.Wrap,
    since cosmos-sdk already support extract stacktrace in debug mode.

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 changed the title fix error message in SendTransaction json-rpc api improve error message in SendTransaction json-rpc api Nov 26, 2021
@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #786 (a8a94da) into main (32eaec8) will decrease coverage by 0.15%.
The diff coverage is 52.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
- Coverage   57.45%   57.30%   -0.16%     
==========================================
  Files          71       71              
  Lines        6107     6041      -66     
==========================================
- Hits         3509     3462      -47     
+ Misses       2396     2377      -19     
  Partials      202      202              
Impacted Files Coverage Δ
app/ante/ante.go 49.38% <0.00%> (+1.18%) ⬆️
app/export.go 13.38% <0.00%> (-0.10%) ⬇️
client/testnet.go 0.00% <0.00%> (ø)
x/evm/keeper/grpc_query.go 66.09% <0.00%> (ø)
x/evm/keeper/keeper.go 75.86% <0.00%> (ø)
x/evm/keeper/msg_server.go 73.58% <0.00%> (ø)
x/evm/keeper/state_transition.go 76.97% <23.52%> (+0.07%) ⬆️
app/ante/eth.go 85.10% <72.72%> (+2.81%) ⬆️
x/evm/keeper/utils.go 72.85% <72.72%> (-2.15%) ⬇️
x/evm/types/chain_config.go 100.00% <100.00%> (ø)
... and 1 more

if err == nil {
err = errors.New(rsp.RawLog)
}
if rsp != nil && rsp.Code != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

&& is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it try to recover error from rsp if rsp.Code != 0.

if err == nil {
err = errors.New(rsp.RawLog)
}
if rsp != nil && rsp.Code != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

&& is correct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -28,7 +27,7 @@ func (k Keeper) DeductTxCostsFromUserBalance(
// fetch sender account from signature
signerAcc, err := authante.GetSignerAcc(ctx, k.accountKeeper, msgEthTx.GetFrom())
if err != nil {
return nil, stacktrace.Propagate(err, "account not found for sender %s", msgEthTx.From)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

how about adding additional info
account not found for sender..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, kept original error message with sdkerrors.Wrapf.

"failed to deduct full gas cost %s from the user %s balance",
fees, msgEthTx.From,
)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

adding addtional info
failed to deduct full gas cost %s from the user %s balance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kept original error message with sdkerrors.Wrapf.

Closes: evmos#785

Solution:
- Remove `stacktrace.Propagate`s, and recover error message in jsonrpc server

changelog

fix error messages
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.

ACK

x/evm/keeper/msg_server.go Outdated Show resolved Hide resolved
@fedekunze fedekunze added backport/0.16.x PR scheduled for inclusion in the v0.16's next stable release backport/0.17.x PR scheduled for inclusion in the v0.17's next stable release labels Nov 26, 2021
@fedekunze fedekunze merged commit c8d4d3f into evmos:main Nov 26, 2021
mergify bot pushed a commit that referenced this pull request Nov 26, 2021
* fix error message in `SendTransaction` json-rpc api

Closes: #785

Solution:
- Remove `stacktrace.Propagate`s, and recover error message in jsonrpc server

changelog

fix error messages

* Update x/evm/keeper/msg_server.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit c8d4d3f)

# Conflicts:
#	CHANGELOG.md
mergify bot pushed a commit that referenced this pull request Nov 26, 2021
* fix error message in `SendTransaction` json-rpc api

Closes: #785

Solution:
- Remove `stacktrace.Propagate`s, and recover error message in jsonrpc server

changelog

fix error messages

* Update x/evm/keeper/msg_server.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit c8d4d3f)

# Conflicts:
#	CHANGELOG.md
#	app/ante/eth.go
#	go.mod
#	rpc/ethereum/namespaces/eth/api.go
#	x/evm/keeper/keeper.go
#	x/evm/keeper/state_transition.go
@yihuang yihuang deleted the error-message branch November 27, 2021 02:08
thomas-nguy referenced this pull request in crypto-org-chain/ethermint Nov 29, 2021
thomas-nguy referenced this pull request in crypto-org-chain/ethermint Nov 29, 2021
leejw51crypto pushed a commit to leejw51crypto/ethermint that referenced this pull request Nov 30, 2021
* fix error message in `SendTransaction` json-rpc api

Closes: evmos#785

Solution:
- Remove `stacktrace.Propagate`s, and recover error message in jsonrpc server

changelog

fix error messages

* Update x/evm/keeper/msg_server.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/0.16.x PR scheduled for inclusion in the v0.16's next stable release backport/0.17.x PR scheduled for inclusion in the v0.17's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON-RPC SendTransaction's error message is not informative
3 participants