From 400999361043feac02c98c369d2e9b1b712250e6 Mon Sep 17 00:00:00 2001 From: Lukasz Cwik Date: Tue, 22 Aug 2023 14:01:07 -0700 Subject: [PATCH] Address https://github.com/dydxprotocol/cosmos-sdk/pull/16 PR comments --- README.md | 7 ++++--- baseapp/baseapp.go | 14 ++++++-------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 94764c3a14f1..8ff7f93bd0e0 100644 --- a/README.md +++ b/README.md @@ -32,9 +32,10 @@ When a new version of CosmosSDK is published, we may want to adopt the changes i 3. Push the new branch (i.e `dydx-fork-v0.47.1`). 4. Off of this new branch, create a new branch. (i.e `totoro/dydxCommits`) 5. Cherry-pick each dydx-created commit from the current default branch, in order, on to the new `dydx-fork-$VERSION` branch (note: you may want to consider creating multiple PRs for this process if there are difficulties or merge conflicts). For example, `git cherry-pick `. You can verify the first commit by seeing the most recent commit sha for the `$VERSION` (i.e `v0.47.1`) tag on the `cosmos/cosmos-sdk` repo, and taking the next commit from that sha. -6. Open a PR to merge the second branch (`totoro/dydxCommits`) into the first (`dydx-fork-v0.47.1`). Get approval, and merge. *DO NOT SQUASH AND MERGE. PLEASE REBASE AND MERGE* -7. Update `dydxprotocol/v4` by following the steps in "Making Changes to the fork" above. -8. Set `dydx-fork-$VERSION` as the [default branch](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-branches-in-your-repository/changing-the-default-branch) in this repository. +6. The dYdX fork uses an optimized version of `runTx` for handling ABCI CheckTx messages. Ensure that the logic within `runCheckTxConcurrently` stays up to date with the `runTx` counterpart by diffing [baseapp.go#runTx](https://github.com/dydxprotocol/cosmos-sdk/blob/6477dcf5693422fef693432e29bd979709aa995d/baseapp/baseapp.go#L754) current version against the new version copying over relevant changes to [baseapp.go#runCheckTxConcurrently](https://github.com/dydxprotocol/cosmos-sdk/blob/6477dcf5693422fef693432e29bd979709aa995d/baseapp/baseapp.go#L619). +7. Open a PR to merge the second branch (`totoro/dydxCommits`) into the first (`dydx-fork-v0.47.1`). Get approval, and merge. *DO NOT SQUASH AND MERGE. PLEASE REBASE AND MERGE* +8. Update `dydxprotocol/v4` by following the steps in "Making Changes to the fork" above. +9. Set `dydx-fork-$VERSION` as the [default branch](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-branches-in-your-repository/changing-the-default-branch) in this repository. Note that the github CI workflows need permissioning. If they fail with a permissioning error, i.e `Resource not accessible through integration`, a Github admin will need to whitelist the workflow. diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 7210793d67ab..5e5b7b833548 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -621,7 +621,9 @@ func (app *BaseApp) runCheckTxConcurrently(mode runTxMode, txBytes []byte) (gInf panic("runCheckTxConcurrently can only be invoked for CheckTx and RecheckTx.") } - // Strip out the post handler + // We don't support the post handler specifically to avoid creating a branched MultiStore and since dYdX + // doesn't need support for it. Once support is necessary or when we are trying to upstream these changes + // we can guard creation of the MultiStore to only occur when the post handler is specified. if app.postHandler != nil { panic("CheckTx/RecheckTx does not support a post hander.") } @@ -729,18 +731,14 @@ func (app *BaseApp) runCheckTxConcurrently(mode runTxMode, txBytes []byte) (gInf return gInfo, result, anteEvents, priority, sdkerrors.Wrap(err, "failed to marshal tx data") } - msgLogs := make(sdk.ABCIMessageLogs, 0, len(msgs)) result = &sdk.Result{ - Data: data, - Log: strings.TrimSpace(msgLogs.String()), + Data: data, + // Use an empty logs slice and format it to maintain forward compatibility with changes done by the Cosmos SDK. + Log: strings.TrimSpace(sdk.ABCIMessageLogs{}.String()), Events: sdk.EmptyEvents().ToABCIEvents(), MsgResponses: msgResponses, } - // We don't support the post handler specifically to avoid creating a branched MultiStore and since dYdX - // doesn't need support for it. Once support is necessary or when we are trying to upstream these changes - // we can guard creation of the MultiStore to only occur when the post handler is specified. - return gInfo, result, anteEvents, priority, err }