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

Address https://github.com/dydxprotocol/cosmos-sdk/pull/16 PR comments #17

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Address https://github.com/dydxprotocol/cosmos-sdk/pull/16 PR comments #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4009993
Address https://github.com/dydxprotocol/cosmos-sdk/pull/16 PR comments
lcwik Aug 22, 2023
7b12bc9
Address PR comments
lcwik Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
View file
Open in desktop
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
15 changes: 7 additions & 8 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,9 @@
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,15 @@
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()),

Check failure on line 737 in baseapp/baseapp.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

File is not `gofumpt`-ed (gofumpt)
// Use an empty events slice to maintain forward compatibility with changed done by the Cosmos SDK.
Events: sdk.EmptyEvents().ToABCIEvents(),
lcwik marked this conversation as resolved.
Show resolved Hide resolved
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
Loading