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.

Sign up for GitHub

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

Jump to bottom

refactor(baseapp,types)!: msg responses #17348

Merged
merged 10 commits into from
Aug 18, 2023
Merged

refactor(baseapp,types)!: msg responses #17348

merged 10 commits into from
Aug 18, 2023

Conversation

zakir-code
Copy link
Contributor

@zakir-code zakir-code commented Aug 10, 2023 •
edited
Loading

Description

  • The sdk.Result.Data field will be reset here.

    cosmos-sdk/baseapp/baseapp.go

    Lines 981 to 987 in dea5b97

    data, err := makeABCIData(msgResponses)
    if err != nil {
    return nil, errorsmod.Wrap(err, "failed to marshal tx data")
    }
    return &sdk.Result{
    Data: data,

  • msgResult.MsgResponses length must be greater than zero, if there is no error.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@zakir-code zakir-code requested a review from a team as a code owner August 10, 2023 06:09
@zakir-code zakir-code changed the title refactor: delete unused data refactor: msg responses Aug 10, 2023
alexanderbez
alexanderbez reviewed Aug 10, 2023
View reviewed changes
baseapp/baseapp.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt changed the title refactor: msg responses refactor(baseapp,types): msg responses Aug 11, 2023
julienrbrt
julienrbrt reviewed Aug 11, 2023
View reviewed changes
baseapp/baseapp.go Outdated Show resolved Hide resolved
types/result.go Show resolved Hide resolved
@zakir-code
Copy link
Contributor Author

Ok, I've restored the length check.

alexanderbez
alexanderbez approved these changes Aug 12, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@julienrbrt @alexanderbez
Update CHANGELOG.md
c799a28 
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@julienrbrt julienrbrt changed the title refactor(baseapp,types): msg responses refactor(baseapp,types)!: msg responses Aug 12, 2023
julienrbrt
julienrbrt previously requested changes Aug 13, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Can we update this comment:

cosmos-sdk/baseapp/baseapp.go

Line 1001 in 2803afd

// `WrapServiceResult`. We take that Msg response, and aggregate it
and

cosmos-sdk/docs/docs/building-modules/03-msg-services.md

Line 95 in 2803afd

The invoked `msgServer` method returns a `proto.Message` response and an `error`. These return values are then wrapped into an `*sdk.Result` or an `error` using `sdk.WrapServiceResult(ctx context.Context, res proto.Message, err error)`:

@zakir-code
Copy link
Contributor Author

@julienrbrt sorry for the late reply.

@julienrbrt julienrbrt dismissed their stale review August 16, 2023 15:07

feedback implemented

julienrbrt
julienrbrt reviewed Aug 16, 2023
View reviewed changes
docs/docs/building-modules/03-msg-services.md
@@ -92,7 +92,7 @@ ctx.EventManager().EmitEvent(

These events are relayed back to the underlying consensus engine and can be used by service providers to implement services around the application. Click [here](../core/08-events.md) to learn more about events.

The invoked `msgServer` method returns a `proto.Message` response and an `error`. These return values are then wrapped into an `*sdk.Result` or an `error` using `sdk.WrapServiceResult(ctx context.Context, res proto.Message, err error)`:
The invoked `msgServer` method returns a `proto.Message` response and an `error`. These return values are then wrapped into an `*sdk.Result` or an `error`:

```go reference
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-alpha.0/baseapp/msg_service_router.go#L160
Copy link
Member

Choose a reason for hiding this comment

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

The snippet here has to change in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't understand what you mean? Do you need to revert the changes here?

@julienrbrt julienrbrt added this pull request to the merge queue Aug 16, 2023
@julienrbrt julienrbrt removed this pull request from the merge queue due to a manual request Aug 16, 2023
julienrbrt
julienrbrt reviewed Aug 16, 2023
View reviewed changes
CHANGELOG.md
@@ -121,6 +121,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/genutil) [#17098](https://github.com/cosmos/cosmos-sdk/pull/17098) `GenAppStateFromConfig`, AddGenesisAccountCmd and `GenTxCmd` takes an addresscodec to decode addresses
* (x/distribution) [#17098](https://github.com/cosmos/cosmos-sdk/pull/17098) `NewMsgDepositValidatorRewardsPool`, `NewMsgFundCommunityPool`, `NewMsgWithdrawValidatorCommission` and `NewMsgWithdrawDelegatorReward` takes a string instead of `sdk.ValAddress` or `sdk.AccAddress`
* (client) [#17215](https://github.com/cosmos/cosmos-sdk/pull/17215) `server.StartCmd`,`server.ExportCmd`,`server.NewRollbackCmd`,`pruning.Cmd`,`genutilcli.InitCmd`,`genutilcli.GenTxCmd`,`genutilcli.CollectGenTxsCmd`,`genutilcli.AddGenesisAccountCmd`, do not take a home directory anymore. It is inferred from the root command.
* (types) [#17348](https://github.com/cosmos/cosmos-sdk/pull/17348) Remove the `WrapServiceResult` function.
Copy link
Member

@julienrbrt julienrbrt Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can we explain the behavior change as well, if someone is calling a msg the msg server router directly and expects the .Data (even if it is deprecated) field to be filled that won't be there.

@julienrbrt julienrbrt added this pull request to the merge queue Aug 18, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 18, 2023
@julienrbrt julienrbrt added this pull request to the merge queue Aug 18, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 18, 2023
@julienrbrt julienrbrt added this pull request to the merge queue Aug 18, 2023
Merged via the queue into cosmos:main with commit 30997f4 Aug 18, 2023
39 of 44 checks passed
samricotta pushed a commit that referenced this pull request Aug 19, 2023
@zakir-code @julienrbrt
@alexanderbez @samricotta
refactor(baseapp,types)!: msg responses (#17348)
42080b8 
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
samricotta added a commit that referenced this pull request Aug 21, 2023
@samricotta
Revert "refactor(baseapp,types)!: msg responses (#17348)"
88ede38 
This reverts commit 42080b8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienrbrt julienrbrt julienrbrt approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@likhita-809 likhita-809 Awaiting requested review from likhita-809

@facundomedica facundomedica Awaiting requested review from facundomedica facundomedica is a code owner

Assignees

@alexanderbez alexanderbez

@julienrbrt julienrbrt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zakir-code @alexanderbez @julienrbrt