Skip to content

Commit

Permalink
Address #16 PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
lcwik committed Aug 22, 2023
1 parent 0324666 commit 4009993
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 11 deletions.
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <commit hash>`. 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.

Expand Down
14 changes: 6 additions & 8 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
}
Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit 4009993

Please sign in to comment.