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

The omission of codespaces when BroadcastTxSync #5859

Closed
whylee259 opened this issue Mar 24, 2020 · 6 comments · Fixed by #6195
Closed

The omission of codespaces when BroadcastTxSync #5859

whylee259 opened this issue Mar 24, 2020 · 6 comments · Fixed by #6195

Comments

@whylee259
Copy link
Contributor

whylee259 commented Mar 24, 2020

func NewResponseFormatBroadcastTx(res *ctypes.ResultBroadcastTx) TxResponse {

When the broadcast mode SYNC, the codespace field is omitted.
Is it intended?

If the failure is from the module, such as msg.ValidateBasic(), the codespace is significant for recognizing the errors.

@alexanderbez
Copy link
Contributor

Messages are not executed during sync/async -- only the ante-handler. Thus it makes no sense to have a codespace. You'll notice, ResultBroadcastTx doesn't have a codespace field.

@whylee259
Copy link
Contributor Author

@alexanderbez

@alexanderbez
Copy link
Contributor

alexanderbez commented Mar 25, 2020

Right, so I'm having trouble following the proposal exactly...?

@whylee259
Copy link
Contributor Author

whylee259 commented Mar 26, 2020

I mean,

The codespace is also needed to SYNC mode.
If an error occurs in validateBasicTxMsgs, codespace + code guarantees the error uniqueness.

I made an example as below.

  • cosmos-sdk/MsgCreateValidator
  • set commission.rate to -1
  • then broadcast sync response is
    {
      "height": "0",
      "txhash": "DF2FC489D5EA6F60FA0DC651A5D2C3F62A7947C2845AA608F1C176D80DD5B6BE",
      "code": 9,
      "raw_log": "commission must be positive"
    }

the code 9 is used by

  • ErrUnknownAddress
  • ErrInvalidProposalAmount
  • ErrCommissionNegative

Clients can not recognize the exact error by code only.

@alexanderbez
Copy link
Contributor

Totally, I agree! But wouldn't ResultBroadcastTx need to support and include codespace? If so, that's a change required in Tendermint first. Then we can support it here 👍

@whylee259
Copy link
Contributor Author

whylee259 commented May 11, 2020

codespace was added to ResultBroadcastTx in the latest tendermint version(v0.33.4)

https://github.com/tendermint/tendermint/blob/master/CHANGELOG.md#features

Could you update the issue status?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants