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

feat: simulate nested messages #20291

Merged
merged 18 commits into from
Jul 26, 2024
Merged

Conversation

JulianToledano
Copy link
Contributor

@JulianToledano JulianToledano commented May 6, 2024

Description

Closes:
#15809


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
  • confirmed ! in 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
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • 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 all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Added support for simulating nested messages in the baseapp module of the Cosmos SDK, enhancing transaction complexity handling.
  • Documentation

    • Updated CHANGELOG.md to reflect the new feature for nested message simulation.
    • Revised UPGRADING.md to include detailed information on new gas cost options for nested messages and their practical application.

@JulianToledano JulianToledano requested a review from a team as a code owner May 6, 2024 13:58
Copy link
Contributor

coderabbitai bot commented May 6, 2024

Walkthrough

Walkthrough

The recent changes enhance the baseapp module of the Cosmos SDK by enabling the simulation of nested messages in transactions. This includes modifications to transaction processing, the addition of functions to manage gas costs for nested messages, and new tests for functionality verification. Additionally, the NewSimApp function has been updated to incorporate configurations related to nested messages, ensuring developers can accurately simulate complex transactions.

Changes

File Change Summary
baseapp/abci_test.go Added necessary imports, created the anyMessage function, and implemented the TestABCI_Query_SimulateNestedMessagesTx test function.
baseapp/baseapp.go Introduced new imports, added functions for simulating nested messages, and modified transaction processing logic.
baseapp/options.go Added SetExcludeNestedMsgsGas and SetIncludeNestedMsgsGas functions to manage gas costs for nested messages.
baseapp/utils_test.go Introduced new structs and methods for testing nested messages functionality.
CHANGELOG.md Documented the new feature for simulating nested messages in the baseapp module.
simapp/app.go Updated NewSimApp to include options for handling nested messages in transactions.
simapp/app_di.go Added configuration for excluding specific nested messages from gas simulation in NewSimApp.

Sequence Diagrams

Simulate Nested Messages Flow

sequenceDiagram
    participant User
    participant SimApp
    participant BaseApp
    participant Codec

    User->>SimApp: Call NewSimApp with nested messages options
    SimApp->>BaseApp: Initialize with options
    BaseApp->>Codec: Process nested messages
    Codec-->>BaseApp: Return processed messages
    BaseApp-->>SimApp: Initialization complete
    
    User->>BaseApp: Submit transaction with nested messages
    BaseApp->>BaseApp: simulateNestedMessages
    BaseApp-->>User: Return simulation result
Loading

Transaction Processing with Nested Messages

sequenceDiagram
    participant User
    participant BaseApp
    participant MsgHandler
    participant GasMeter

    User->>BaseApp: Submit transaction
    BaseApp->>BaseApp: runTx
    BaseApp->>MsgHandler: Process nested messages
    MsgHandler-->>BaseApp: Return processed results
    BaseApp->>GasMeter: Calculate gas for nested messages
    GasMeter-->>BaseApp: Return gas info
    BaseApp-->>User: Return transaction result
Loading

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

This comment has been minimized.

@JulianToledano JulianToledano linked an issue May 6, 2024 that may be closed by this pull request
@@ -810,6 +812,10 @@ func (app *BaseApp) endBlock(ctx context.Context) (sdk.EndBlock, error) {
return endblock, nil
}

type HasNestedMsgs interface {
GetMsgs() ([]sdk.Msg, error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whats the best place to define this 🤨

Copy link
Contributor

Choose a reason for hiding this comment

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

it is a good practice to define the type where it is used. You can also consider making it private or inline it into the function where it is used. In this case there is the same interface in types called HasMsgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm HasMsgs is pretty similar but doesn't return an error. I think it was designed with tx in mind.

GetMsgs() []Msg

Makes me wonder if we should update HasMsgs or stick with this 'duplicate'. If we opt for both interfaces, baseapp is not the ideal place for HasNestedMsgs as it may be useful for ante handlers.
@tac0turtle @julienrbrt

Copy link
Member

Choose a reason for hiding this comment

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

what is the possible error that would be returned here? feel like it wouldnt return an error and just be length of 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opted for that because current messages with nested messages within the SDK have defined that method:

// GetMessages returns the proposal messages
func (p Proposal) GetMsgs() ([]sdk.Msg, error) {
return sdktx.GetMsgs(p.Messages, "sdk.MsgProposal")
}

// GetMsgs unpacks p.Messages Any's into sdk.Msg's
func (p *Proposal) GetMsgs() ([]sdk.Msg, error) {
return tx.GetMsgs(p.Messages, "proposal")
}

I guess we should standardise this and document it.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Out of diff range and nitpick comments (4)
baseapp/testutil/messages.proto (2)

37-43: Ensure the MsgSend message fields are adequately documented for better maintainability.


47-52: Consider adding documentation for the MsgNestedMessages message to explain the purpose and usage of each field.

baseapp/baseapp.go (2)

16-16: Consider organizing imports to improve readability.


20-20: Consider organizing imports to improve readability.

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 89ccbc1 and cc1efda.
Files ignored due to path filters (1)
  • baseapp/testutil/messages.pb.go is excluded by !**/*.pb.go
Files selected for processing (5)
  • baseapp/abci_test.go (2 hunks)
  • baseapp/baseapp.go (4 hunks)
  • baseapp/testutil/messages.go (3 hunks)
  • baseapp/testutil/messages.proto (2 hunks)
  • baseapp/utils_test.go (2 hunks)
Additional Context Used
Path-based Instructions (4)
baseapp/testutil/messages.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

baseapp/utils_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

baseapp/baseapp.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

baseapp/abci_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (6)
baseapp/testutil/messages.proto (2)

68-70: LGTM! The Send service is correctly defined to handle MsgSend.


72-74: LGTM! The NestedMessages service is correctly defined to handle MsgNestedMessages.

baseapp/testutil/messages.go (2)

20-21: LGTM! The new messages MsgNestedMessages and MsgSend are correctly registered.


27-28: LGTM! The message service descriptions for MsgNestedMessages and MsgSend are correctly registered.

baseapp/baseapp.go (2)

815-817: Ensure all implementations of HasNestedMsgs properly handle errors returned by GetMsgs().


Line range hint 1-1124: The structure and formatting of the file are consistent and well-integrated with the existing code.

baseapp/testutil/messages.go Show resolved Hide resolved
baseapp/utils_test.go Outdated Show resolved Hide resolved
baseapp/utils_test.go Outdated Show resolved Hide resolved
baseapp/abci_test.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between cc1efda and 9556fe5.
Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 9556fe5 and 9409167.
Files selected for processing (2)
  • baseapp/abci_test.go (2 hunks)
  • baseapp/utils_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • baseapp/abci_test.go
  • baseapp/utils_test.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 9409167 and 267b4b4.
Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Additional comments not posted (1)
CHANGELOG.md (1)

45-45: Ensure the entry for the nested messages feature is clear and concise.

This entry correctly links the PR and describes the feature added. It's important for tracking changes and functionalities introduced in the system.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 267b4b4 and c0b2318.
Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • baseapp/abci_test.go (2 hunks)
  • baseapp/baseapp.go (4 hunks)
  • baseapp/utils_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • baseapp/baseapp.go
  • baseapp/utils_test.go
Additional Context Used
GitHub Check Runs (1)
golangci-lint failure (3)

baseapp/abci_test.go: [failure] 773-773:
undefined: abci.RequestInitChain (typecheck)

Path-based Instructions (2)
baseapp/abci_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Additional comments not posted (1)
CHANGELOG.md (1)

45-45: The changelog entry for simulating nested messages is clear and correctly linked to the PR.

baseapp/abci_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Good start! 🏄
I added some nits and personal preference but I noticed that I have some context missing for a proper review. Are the other PRs coming for check and deliver TX?

wantErr bool
}{
{
name: "ok nested message",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you use a map[string]struct{} for the tests spec, you can avoid the name attribute and use the map key instead

@@ -810,6 +812,10 @@ func (app *BaseApp) endBlock(ctx context.Context) (sdk.EndBlock, error) {
return endblock, nil
}

type HasNestedMsgs interface {
GetMsgs() ([]sdk.Msg, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

it is a good practice to define the type where it is used. You can also consider making it private or inline it into the function where it is used. In this case there is the same interface in types called HasMsgs.

baseapp/baseapp.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between c0b2318 and e9569ed.
Files selected for processing (2)
  • baseapp/abci_test.go (2 hunks)
  • baseapp/baseapp.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
  • baseapp/abci_test.go
  • baseapp/baseapp.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between e9569ed and 6d0e987.
Files selected for processing (2)
  • baseapp/abci_test.go (3 hunks)
  • baseapp/baseapp.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
  • baseapp/abci_test.go
  • baseapp/baseapp.go

@JulianToledano
Copy link
Contributor Author

Are the other PRs coming for check and deliver TX?

I don't think there'll be more prs. This is just for simulation which just runs runTx

// Simulate executes a tx in simulate mode to get result and gas info.
func (app *BaseApp) Simulate(txBytes []byte) (sdk.GasInfo, *sdk.Result, error) {
gasInfo, result, _, err := app.runTx(execModeSimulate, txBytes)
return gasInfo, result, err
}

baseapp/baseapp.go Outdated Show resolved Hide resolved
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Jun 22, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Outside diff range and nitpick comments (2)
baseapp/baseapp.go (1)

817-819: Interface Definition for Nested Messages.

The HasNestedMsgs interface is well-defined and serves its purpose for objects that can return nested messages. However, consider merging this functionality with the existing HasMsgs interface if they are sufficiently similar to avoid redundancy.

baseapp/abci_test.go (1)

767-775: Consider using a helper function for error handling.

The function anyMessage marshals a message and directly uses require.NoError for error handling. This approach is fine for tests, but it could be improved by handling the error more gracefully or by using a helper function that returns an error which can then be handled appropriately in the calling test case.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6d0e987 and c6a8a61.

Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • baseapp/abci_test.go (3 hunks)
  • baseapp/baseapp.go (4 hunks)
Additional context used
Path-based instructions (3)
baseapp/baseapp.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

baseapp/abci_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

GitHub Check: tests (03)
baseapp/baseapp.go

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: tests (02)
baseapp/baseapp.go

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: tests (01)
baseapp/baseapp.go

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: tests (00)
baseapp/baseapp.go

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: golangci-lint
baseapp/baseapp.go

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs) (typecheck)


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs) (typecheck)


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs) (typecheck)

GitHub Check: dependency-review
baseapp/baseapp.go

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

golangci-lint
baseapp/abci_test.go

20-20: abci redeclared in this block (typecheck)

Markdownlint
CHANGELOG.md

73-73: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


74-74: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


78-78: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


79-79: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


80-80: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


81-81: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


86-86: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


129-129: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


130-130: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


131-131: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


135-135: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


138-138: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


139-139: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


140-140: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


147-147: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


157-157: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


159-159: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


162-162: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


181-181: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


182-182: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


184-184: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


185-185: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


237-237: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


238-238: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


239-239: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


403-403: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


406-406: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


428-428: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


429-429: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


442-442: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


474-474: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


475-475: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


476-476: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


477-477: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


479-479: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


480-480: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


481-481: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


482-482: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


496-496: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


498-498: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


500-500: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


502-502: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


505-505: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


506-506: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


507-507: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


515-515: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


516-516: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


518-518: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


519-519: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


521-521: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


522-522: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


523-523: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


525-525: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


526-526: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


534-534: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


545-545: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


546-546: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


547-547: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


553-553: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


554-554: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


555-555: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


561-561: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


577-577: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


578-578: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


579-579: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


580-580: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


581-581: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


582-582: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


587-587: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


588-588: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


589-589: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


590-590: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


597-597: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


598-598: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


599-599: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


633-633: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


634-634: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


635-635: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


636-636: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


641-641: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


642-642: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


790-790: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


933-933: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


954-954: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


957-957: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1039-1039: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1040-1040: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1041-1041: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1042-1042: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1043-1043: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1044-1044: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1141-1141: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1227-1227: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1273-1273: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1279-1279: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1280-1280: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1281-1281: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1282-1282: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1283-1283: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1284-1284: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1384-1384: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1509-1509: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1510-1510: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1511-1511: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1512-1512: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1513-1513: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1514-1514: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1515-1515: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1516-1516: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1519-1519: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1520-1520: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1521-1521: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1522-1522: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1523-1523: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1524-1524: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


1773-1773: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1774-1774: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1775-1775: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1776-1776: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1777-1777: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1778-1778: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


1888-1888: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2225-2225: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2226-2226: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2227-2227: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2230-2230: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2231-2231: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2232-2232: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2254-2254: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2255-2255: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2256-2256: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2257-2257: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2258-2258: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2266-2266: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2267-2267: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2268-2268: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2269-2269: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2270-2270: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2272-2272: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2273-2273: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2274-2274: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2601-2601: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2602-2602: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2603-2603: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2604-2604: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2605-2605: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2607-2607: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2609-2609: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2610-2610: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2611-2611: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2612-2612: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2613-2613: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2614-2614: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2616-2616: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2617-2617: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2618-2618: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2621-2621: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2622-2622: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2623-2623: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2624-2624: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2625-2625: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2628-2628: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2631-2631: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2634-2634: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2635-2635: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2638-2638: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2645-2645: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2646-2646: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2647-2647: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2648-2648: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2649-2649: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2651-2651: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2652-2652: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2653-2653: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2654-2654: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2655-2655: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2656-2656: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2657-2657: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2658-2658: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2659-2659: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2662-2662: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2663-2663: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2664-2664: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2665-2665: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2666-2666: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2667-2667: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2674-2674: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2675-2675: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2676-2676: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2677-2677: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2684-2684: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2686-2686: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2688-2688: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2689-2689: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2690-2690: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2691-2691: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2692-2692: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2693-2693: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2694-2694: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2695-2695: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2696-2696: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2697-2697: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2698-2698: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2699-2699: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2700-2700: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2701-2701: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2702-2702: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2703-2703: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2704-2704: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2705-2705: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2706-2706: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2707-2707: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2708-2708: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2709-2709: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2710-2710: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2711-2711: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2712-2712: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2713-2713: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2714-2714: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2716-2716: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2717-2717: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2719-2719: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2720-2720: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2721-2721: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2722-2722: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2723-2723: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2724-2724: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2725-2725: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2728-2728: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2729-2729: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2731-2731: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2732-2732: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2735-2735: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2736-2736: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2737-2737: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2738-2738: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2739-2739: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2740-2740: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2741-2741: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2742-2742: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2744-2744: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2745-2745: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2746-2746: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2752-2752: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2755-2755: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2761-2761: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2769-2769: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2770-2770: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2771-2771: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2772-2772: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2780-2780: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2787-2787: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2788-2788: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2795-2795: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2797-2797: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2801-2801: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2802-2802: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2804-2804: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2812-2812: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2814-2814: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2815-2815: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2821-2821: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2829-2829: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2830-2830: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2831-2831: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2832-2832: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2833-2833: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2834-2834: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2835-2835: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2836-2836: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2837-2837: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2838-2838: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2839-2839: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2840-2840: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2841-2841: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2842-2842: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2844-2844: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2845-2845: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2847-2847: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2848-2848: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2849-2849: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2850-2850: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2851-2851: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2852-2852: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2853-2853: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2854-2854: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2855-2855: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2856-2856: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2858-2858: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2859-2859: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2862-2862: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2863-2863: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2864-2864: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2865-2865: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2866-2866: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2867-2867: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2868-2868: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2869-2869: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2870-2870: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2871-2871: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2872-2872: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2873-2873: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2874-2874: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2875-2875: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2876-2876: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2877-2877: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2882-2882: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2883-2883: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2884-2884: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2885-2885: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2886-2886: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2887-2887: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2889-2889: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2891-2891: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2905-2905: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2906-2906: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2907-2907: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2912-2912: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2913-2913: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2914-2914: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2918-2918: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2919-2919: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2920-2920: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2921-2921: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2922-2922: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2923-2923: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2926-2926: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2927-2927: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2928-2928: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2929-2929: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2930-2930: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2931-2931: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2932-2932: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2933-2933: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2934-2934: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2936-2936: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2938-2938: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2940-2940: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2945-2945: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2946-2946: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2947-2947: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2948-2948: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2949-2949: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2950-2950: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2951-2951: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2952-2952: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2953-2953: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2954-2954: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2955-2955: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2956-2956: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2957-2957: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2958-2958: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2959-2959: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2960-2960: Expected: 4; Actual: 8 (MD007, ul-indent)
Unordered list indentation


2961-2961: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2962-2962: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2963-2963: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2964-2964: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


2965-2965: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation


191-191: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces


234-234: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces


1143-1143: null (MD024, no-duplicate-heading)
Multiple headings with the same content


1933-1933: null (MD024, no-duplicate-heading)
Multiple headings with the same content


1691-1691: null (MD034, no-bare-urls)
Bare URL used


1721-1721: null (MD034, no-bare-urls)
Bare URL used


2668-2668: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2756-2756: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2758-2758: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2763-2763: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2765-2765: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2775-2775: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2777-2777: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2782-2782: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2790-2790: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2807-2807: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2809-2809: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2817-2817: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2823-2823: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2893-2893: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2896-2896: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2899-2899: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2901-2901: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


2941-2941: null (MD037, no-space-in-emphasis)
Spaces inside emphasis markers


1062-1062: null (MD038, no-space-in-code)
Spaces inside code span elements


2709-2709: null (MD038, no-space-in-code)
Spaces inside code span elements


2709-2709: null (MD038, no-space-in-code)
Spaces inside code span elements

Additional comments not posted (7)
baseapp/baseapp.go (2)

16-20: Ensure appropriate usage of new imports.

The new imports (anyutil, proto, anypb) have been added to support the handling of nested messages and their serialization. It's crucial to ensure that these libraries are used securely and efficiently throughout the codebase.


1081-1111: Nested Message Simulation Logic

The method simulateNestedMessages recursively simulates nested messages. This is a critical piece of functionality for the feature introduced in this PR. Ensure that the recursion does not lead to performance issues or stack overflows with deeply nested structures.

Tools
GitHub Check: tests (03)

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: tests (02)

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: tests (01)

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: tests (00)

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

GitHub Check: golangci-lint

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs) (typecheck)


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs) (typecheck)


[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs) (typecheck)

GitHub Check: dependency-review

[failure] 1109-1109:
cannot use msgsV2 (variable of type []protoreflect.ProtoMessage) as []protoreflect.Message value in argument to app.runMsgs

baseapp/abci_test.go (1)

777-923: Review the use of table-driven tests for clarity and maintainability.

The test function TestABCI_Query_SimulateNestedMessagesTx uses a table-driven approach, which is good for maintainability. However, ensure that the test cases cover all relevant scenarios for nested messages, including edge cases. Additionally, consider adding more detailed inline comments explaining the purpose of each test case, especially for complex scenarios like nested messages.

CHANGELOG.md (4)

45-45: Entry looks good!

The changelog entry for the baseapp feature correctly references the PR and describes the new functionality.


45-45: Entry looks good!

The changelog entry for the client feature is clear and correctly linked to its PR.


45-45: Entry looks good!

The changelog entry for the tests feature is appropriately detailed and linked.


45-45: Entry looks good!

The changelog entry for the runtime feature is clear and correctly linked to its PR.

baseapp/abci_test.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label Jun 23, 2024
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I think the gas refund may create some issues with the current implementatio.

baseapp/baseapp.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Show resolved Hide resolved
simapp/app.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c035d34 and 6260cfd.

Files selected for processing (4)
  • CHANGELOG.md (1 hunks)
  • baseapp/baseapp.go (5 hunks)
  • simapp/app.go (1 hunks)
  • simapp/app_di.go (2 hunks)
Additional context used
Path-based instructions (4)
simapp/app_di.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simapp/app.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

baseapp/baseapp.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Additional comments not posted (5)
simapp/app_di.go (1)

243-244: Verify the addition of SetExcludeNestedMsgsGas function.

The SetExcludeNestedMsgsGas function has been added to baseAppOptions. Ensure that this function is correctly implemented and integrated.

Verification successful

The SetExcludeNestedMsgsGas function is correctly implemented and integrated.

The SetExcludeNestedMsgsGas function is defined in baseapp/options.go and is used appropriately in simapp/app_di.go, simapp/app.go, and other relevant files. The review comment regarding its addition to baseAppOptions is accurate.

  • Implementation found in: baseapp/options.go
  • Usage found in: simapp/app_di.go, simapp/app.go, baseapp/abci_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of `SetExcludeNestedMsgsGas` function.

# Test: Search for the function implementation. Expect: Correct implementation and integration.
rg --type go --pattern 'func SetExcludeNestedMsgsGas'

Length of output: 467


Script:

#!/bin/bash
# Description: Verify the implementation and usage of `SetExcludeNestedMsgsGas` function.

# Test: Search for the function implementation. Expect: Correct implementation and integration.
rg 'func SetExcludeNestedMsgsGas'

# Test: Search for the usage of `SetExcludeNestedMsgsGas` in the codebase.
rg 'SetExcludeNestedMsgsGas'

Length of output: 728

simapp/app.go (1)

249-250: Verify the addition of SetExcludeNestedMsgsGas function.

The SetExcludeNestedMsgsGas function has been added to baseAppOptions. Ensure that this function is correctly implemented and integrated.

Verification successful

Verify the addition of SetExcludeNestedMsgsGas function.

The SetExcludeNestedMsgsGas function is correctly implemented in baseapp/options.go and properly integrated into baseAppOptions.

  • Implementation of SetExcludeNestedMsgsGas found in baseapp/options.go:
    func SetExcludeNestedMsgsGas(msgs []sdk.Msg) func(*BaseApp) {
        return func(app *BaseApp) {
            app.excludeNestedMsgsGas = make(map[string]struct{})
            for _, msg := range msgs {
                if _, ok := msg.(HasNestedMsgs); !ok {
                    continue
                }
                app.excludeNestedMsgsGas[sdk.MsgTypeURL(msg)] = struct{}{}
            }
        }
    }
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of `SetExcludeNestedMsgsGas` function.

# Test: Search for the function implementation. Expect: Correct implementation and integration.
rg --type go --pattern 'func SetExcludeNestedMsgsGas'

Length of output: 467


Script:

#!/bin/bash
# Description: Verify the implementation of `SetExcludeNestedMsgsGas` function.

# Test: Search for the function implementation. Expect: Correct implementation and integration.
rg 'func SetExcludeNestedMsgsGas' --type go

Length of output: 126


Script:

#!/bin/bash
# Description: Retrieve the full implementation of the `SetExcludeNestedMsgsGas` function.

# Test: Extract the function implementation. Expect: Complete function implementation.
rg 'func SetExcludeNestedMsgsGas' -A 20 --type go baseapp/options.go

Length of output: 556

baseapp/baseapp.go (2)

239-241: LGTM! The initialization of excludeNestedMsgsGas is correct.

The added code ensures that the excludeNestedMsgsGas map is initialized if it is nil, preventing potential nil pointer dereference errors.


967-978: LGTM! But verify the function usage in the codebase.

The code changes are approved. The added code correctly simulates nested messages and handles gas refunds during the simulation mode.

However, ensure that the simulateNestedMessages function is used appropriately in the codebase.

Verification successful

Function usage verified

The function simulateNestedMessages is used appropriately within the codebase.

  • baseapp/baseapp.go: The function is defined and used within the same file.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `simulateNestedMessages` function.

# Test: Search for the function usage. Expect: Occurrences in appropriate contexts.
rg --type go $'simulateNestedMessages'

Length of output: 355

CHANGELOG.md (1)

45-45: Correctness of the changelog entry.

The changelog entry for simulating nested messages is correct and follows the format used for other features.

Comment on lines 1086 to 1120
// simulateNestedMessages simulates a message nested messages.
func (app *BaseApp) simulateNestedMessages(ctx sdk.Context, msg sdk.Msg) error {
nestedMsgs, ok := msg.(HasNestedMsgs)
if !ok {
return nil
}

msgs, err := nestedMsgs.GetMsgs()
if err != nil {
return err
}

for _, msg := range msgs {
err = app.simulateNestedMessages(ctx, msg)
if err != nil {
return err
}
}

if err := validateBasicTxMsgs(app.msgServiceRouter, msgs); err != nil {
return err
}

protoMessages := make([]protoreflect.Message, len(msgs))
for i, msg := range msgs {
_, protoMsg, err := app.cdc.GetMsgSigners(msg)
if err != nil {
return err
}
protoMessages[i] = protoMsg
}

_, err = app.runMsgs(ctx, msgs, protoMessages, execModeSimulate)
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a recursion depth limit or converting to an iterative approach.

The simulateNestedMessages function uses recursion to handle nested messages. The depth of recursion could potentially lead to stack overflow errors with deeply nested structures. Consider adding a recursion depth limit or converting this to an iterative approach using a stack data structure to enhance robustness.

func (app *BaseApp) simulateNestedMessages(ctx sdk.Context, msg sdk.Msg) error {
+  const maxRecursionDepth = 100
+  return app.simulateNestedMessagesHelper(ctx, msg, 0, maxRecursionDepth)
}

+func (app *BaseApp) simulateNestedMessagesHelper(ctx sdk.Context, msg sdk.Msg, depth, maxDepth int) error {
+  if depth > maxDepth {
+    return errors.New("max recursion depth exceeded")
+  }
  nestedMsgs, ok := msg.(HasNestedMsgs)
  if !ok {
    return nil
  }

  msgs, err := nestedMsgs.GetMsgs()
  if err != nil {
    return err
  }

  for _, msg := range msgs {
-    err = app.simulateNestedMessages(ctx, msg)
+    err = app.simulateNestedMessagesHelper(ctx, msg, depth+1, maxDepth)
    if err != nil {
      return err
    }
  }

  if err := validateBasicTxMsgs(app.msgServiceRouter, msgs); err != nil {
    return err
  }

  protoMessages := make([]protoreflect.Message, len(msgs))
  for i, msg := range msgs {
    _, protoMsg, err := app.cdc.GetMsgSigners(msg)
    if err != nil {
      return err
    }
    protoMessages[i] = protoMsg
  }

  _, err = app.runMsgs(ctx, msgs, protoMessages, execModeSimulate)
  return err
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// simulateNestedMessages simulates a message nested messages.
func (app *BaseApp) simulateNestedMessages(ctx sdk.Context, msg sdk.Msg) error {
nestedMsgs, ok := msg.(HasNestedMsgs)
if !ok {
return nil
}
msgs, err := nestedMsgs.GetMsgs()
if err != nil {
return err
}
for _, msg := range msgs {
err = app.simulateNestedMessages(ctx, msg)
if err != nil {
return err
}
}
if err := validateBasicTxMsgs(app.msgServiceRouter, msgs); err != nil {
return err
}
protoMessages := make([]protoreflect.Message, len(msgs))
for i, msg := range msgs {
_, protoMsg, err := app.cdc.GetMsgSigners(msg)
if err != nil {
return err
}
protoMessages[i] = protoMsg
}
_, err = app.runMsgs(ctx, msgs, protoMessages, execModeSimulate)
return err
}
// simulateNestedMessages simulates a message nested messages.
func (app *BaseApp) simulateNestedMessages(ctx sdk.Context, msg sdk.Msg) error {
const maxRecursionDepth = 100
return app.simulateNestedMessagesHelper(ctx, msg, 0, maxRecursionDepth)
}
func (app *BaseApp) simulateNestedMessagesHelper(ctx sdk.Context, msg sdk.Msg, depth, maxDepth int) error {
if depth > maxDepth {
return errors.New("max recursion depth exceeded")
}
nestedMsgs, ok := msg.(HasNestedMsgs)
if (!ok) {
return nil
}
msgs, err := nestedMsgs.GetMsgs()
if (err != nil) {
return err
}
for _, msg := range msgs {
err = app.simulateNestedMessagesHelper(ctx, msg, depth+1, maxDepth)
if (err != nil) {
return err
}
}
if err := validateBasicTxMsgs(app.msgServiceRouter, msgs); err != nil {
return err
}
protoMessages := make([]protoreflect.Message, len(msgs))
for i, msg := range msgs {
_, protoMsg, err := app.cdc.GetMsgSigners(msg)
if (err != nil) {
return err
}
protoMessages[i] = protoMsg
}
_, err = app.runMsgs(ctx, msgs, protoMessages, execModeSimulate)
return err
}

@github-actions github-actions bot removed the C:x/gov label Jul 15, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)
baseapp/options.go (1)

123-133: LGTM! But consider adding a log message for debugging.

The function correctly filters messages that implement the HasNestedMsgs interface. For better debugging, consider adding a log message when a message does not implement the required interface.

+ if _, ok := msg.(HasNestedMsgs); !ok {
+     log.Warn("Message does not implement HasNestedMsgs", "msg", msg)
+     continue
+ }
baseapp/abci_test.go (1)

775-919: Comprehensive test function for nested message simulation.

The function covers various edge cases effectively. Consider using more descriptive names for the test cases to improve readability.

- name: "ok nested message"
+ name: "valid nested message with correct signers and amount"

- name: "different signers"
+ name: "invalid nested message with different signers"

- name: "empty from"
+ name: "invalid nested message with empty from address"

- name: "empty to"
+ name: "invalid nested message with empty to address"

- name: "negative amount"
+ name: "invalid nested message with negative amount"

- name: "with nested messages"
+ name: "valid nested message containing another valid message"

- name: "with invalid nested messages"
+ name: "invalid nested message containing another invalid message"

- name: "with different signer"
+ name: "invalid nested message containing a message with different signer"
Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6260cfd and e5362a5.

Files selected for processing (4)
  • baseapp/abci_test.go (3 hunks)
  • baseapp/baseapp.go (5 hunks)
  • baseapp/options.go (1 hunks)
  • simapp/app_di.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • baseapp/baseapp.go
  • simapp/app_di.go
Additional context used
Path-based instructions (2)
baseapp/options.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

baseapp/abci_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (2)
baseapp/abci_test.go (2)

765-773: Helper function anyMessage is well-implemented.

The function correctly marshals the message and sets the TypeUrl. Ensure that the codec.Codec used handles all necessary message types.


921-997: Well-structured test function for nested message gas simulation.

The function effectively tests different scenarios for gas consumption, including both inclusion and exclusion of nested message gas.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e5362a5 and 79ac6b4.

Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

@JulianToledano JulianToledano requested a review from alpe July 23, 2024 09:07
@@ -186,6 +186,9 @@ type BaseApp struct {
// including the goroutine handling.This is experimental and must be enabled
// by developers.
optimisticExec *oe.OptimisticExecution

// includeNestedMsgsGas holds a set of message types for which gas costs for its nested messages are calculated.
includeNestedMsgsGas map[string]struct{}
Copy link
Member

Choose a reason for hiding this comment

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

can we show an example of this being set?

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

left two comments, otherwise look good to me

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Nice work! 💪

I could not find the integration in simapp to make use of the new feature. Are you planning a new PR?

return func(app *BaseApp) {
app.includeNestedMsgsGas = make(map[string]struct{})
for _, msg := range msgs {
if _, ok := msg.(HasNestedMsgs); !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good to check

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (16)
UPGRADING.md (16)

8-9: Add a blank line after the heading.

To improve readability, add a blank line after the BaseApp heading.

### BaseApp

#### Nested Messages Simulation

10-18: Clarify the limitations of the simulation.

The paragraph explains the limitations of the simulation but could be clearer about the potential discrepancies between simulation and actual execution.

However, it's important to note that while the simulation can provide valuable insights, it does not guarantee the correct execution of the nested messages in the future. Factors such as changes in the blockchain state or updates to the protocol could potentially affect the actual execution of these nested messages when the transaction is finally processed on the network.
+ Therefore, developers should use simulation results as guidance rather than definitive outcomes.

26-31: Clarify the default behavior for gas cost calculation.

The paragraph explains the default behavior and customization options for gas cost calculation. Ensure it is clear and concise.

By default, when simulating transactions, the gas cost of nested messages is not calculated. This means that only the gas cost of the top-level message is considered. However, this behavior can be customized using the `SetIncludeNestedMsgsGas` option when building the BaseApp. By providing a list of message types to this option, you can specify which messages should have their nested message gas costs included in the simulation. This allows for more accurate gas estimation for transactions involving specific message types that contain nested messages, while maintaining the default behavior for other message types.
+ This flexibility enables developers to tailor gas estimation to their specific use cases.

35-39: Surround fenced code block with blank lines.

Ensure the fenced code block is surrounded by blank lines for better readability.

Here is an example on how `SetIncludeNestedMsgsGas` option could be set to calculate the gas of a gov proposal nested messages:

+ ```go
baseAppOptions = append(baseAppOptions, baseapp.SetIncludeNestedMsgsGas([]sdk.Message{&gov.MsgSubmitProposal{}}))
// ...
app.App = appBuilder.Build(db, traceStore, baseAppOptions...)
+ ```
Tools
Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 40-56: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

The `client` package has been refactored to make use of the address codecs (address, validator address, consensus address, etc.)
and address bech32 prefixes (address and validator address).
This is part of the work of abstracting the SDK from the global bech32 config.

This means the address codecs and prefixes must be provided in the `client.Context` in the application client (usually `root.go`).

+ ```diff
clientCtx = clientCtx.
+ WithAddressCodec(addressCodec).
+ WithValidatorAddressCodec(validatorAddressCodec).
+ WithConsensusAddressCodec(consensusAddressCodec).
+ WithAddressPrefix("cosmos").
+ WithValidatorPrefix("cosmosvaloper")
+ ```

**When using `depinject` / `app v2`, the client codecs can be provided directly from application config.**

Refer to SimApp `root_v2.go` and `root.go` for an example with an app v2 and a legacy app.

Additionally, a simplification of the start command leads to the following change:

+ ```diff
- server.AddCommands(rootCmd, newApp, func(startCmd *cobra.Command) {})
+ server.AddCommands(rootCmd, newApp, server.StartCmdOptions[servertypes.Application]{})
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 57-63: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

For non depinject users, simply call `RegisterLegacyAminoCodec` and `RegisterInterfaces` on the module manager:

+ ```diff
-app.BasicModuleManager = module.NewBasicManagerFromManager(...)
-app.BasicModuleManager.RegisterLegacyAminoCodec(legacyAmino)
-app.BasicModuleManager.RegisterInterfaces(interfaceRegistry)
+app.ModuleManager.RegisterLegacyAminoCodec(legacyAmino)
+app.ModuleManager.RegisterInterfaces(interfaceRegistry)
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 72-89: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

To enable unordered transactions in your application:

+ ```go
func NewApp(...) *App {
	// ...

	// create, start, and load the unordered tx manager
	utxDataDir := filepath.Join(cast.ToString(appOpts.Get(flags.FlagHome)), "data")
	app.UnorderedTxManager = unorderedtx.NewManager(utxDataDir)
	app.UnorderedTxManager.Start()

	if err := app.UnorderedTxManager.OnInit(); err != nil {
		panic(fmt.Errorf("failed to initialize unordered tx manager: %w", err))
	}
}
+ ```

* Add the decorator to the existing AnteHandler chain, which should be as early as possible.

+ ```go
anteDecorators := []sdk.AnteDecorator{
	ante.NewSetUpContextDecorator(),
	// ...
	ante.NewUnorderedTxDecorator(unorderedtx.DefaultMaxUnOrderedTTL, app.UnorderedTxManager),
	// ...
}

return sdk.ChainAnteDecorators(anteDecorators...), nil
+ ```

* If the App has a SnapshotManager defined, you must also register the extension for the TxManager.

+ ```go
if manager := app.SnapshotManager(); manager != nil {
	err := manager.RegisterExtensions(unorderedtx.NewSnapshotter(app.UnorderedTxManager))
	if err != nil {
		panic(fmt.Errorf("failed to register snapshot extension: %s", err))
	}
}
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 92-104: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

* Create or update the App's `Close()` method to close the unordered tx manager.
  Note, this is critical as it ensures the manager's state is written to file
  such that when the node restarts, it can recover the state to provide replay
  protection.

+ ```go
func (app *App) Close() error {
	// ...

	// close the unordered tx manager
	if e := app.UnorderedTxManager.Close(); e != nil {
		err = errors.Join(err, e)
	}

	return err
}
+ ```

To submit an unordered transaction, the client must set the `unordered` flag to `true` and ensure a reasonable `timeout_height` is set. The `timeout_height` is used as a TTL for the transaction and is used to provide replay protection. See [ADR-070](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-070-unordered-transactions.md) for more details.
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 105-112: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

It can happen that some modules do not have a store necessary (such as `x/auth/tx` for instance). In this case, the store creation should be skipped in `app_config.go`:

+ ```diff
InitGenesis: []string{
	"..."
},
+ // SkipStoreKeys is an optional list of store keys to skip when constructing the
+ // module's keeper. This is useful when a module does not have a store key.
+ SkipStoreKeys: []string{
+ 	"tx",
+ },
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 113-118: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

The `cosmossdk.io/api/tendermint` package has been removed as CometBFT now publishes its protos to `buf.build/tendermint` and `buf.build/cometbft`.
There is no longer a need for the Cosmos SDK to host these protos for itself and its dependencies.
That package containing proto v2 generated code, but the SDK now uses [buf generated go SDK instead](https://buf.build/docs/bsr/generated-sdks/go).
If you were depending on `cosmossdk.io/api/tendermint`, please use the buf generated go SDK instead, or ask CometBFT host the generated proto v2 code.

+ ```diff
-func (AppModule) RegisterInterfaces(registry codectypes.InterfaceRegistry) {
+func (AppModule) RegisterInterfaces(registry registry.InterfaceRegistrar) {
+ ```

The signature of the extension interface `HasAminoCodec` has been changed to accept a `cosmossdk.io/core/legacy.Amino` instead of a `codec.LegacyAmino`. Modules should update their `HasAminoCodec` implementation to accept a `cosmossdk.io/core/legacy.Amino` interface.

+ ```diff
-func (AppModule) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
+func (AppModule) RegisterLegacyAminoCodec(cdc legacy.Amino) {
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 119-124: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

`MsgSimulatorFn` has been updated to return an error. Its context argument has been removed, and an address.Codec has
been added to avoid the use of the Accounts.String() method.

+ ```diff
-type MsgSimulatorFn func(r *rand.Rand, ctx sdk.Context, accs []Account) sdk.Msg
+type MsgSimulatorFn func(r *rand.Rand, accs []Account, cdc address.Codec) (sdk.Msg, error)
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 125-132: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

Previously `cosmossdk.io/core` held functions `Invoke`, `Provide` and `Register` were moved to `cosmossdk.io/depinject/appconfig`.
All modules using dependency injection must update their imports.

+ ```diff
-type MsgSimulatorFn func(r *rand.Rand, ctx sdk.Context, accs []Account) sdk.Msg
+type MsgSimulatorFn func(r *rand.Rand, accs []Account, cdc address.Codec) (sdk.Msg, error)
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 133-138: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

Previous module migrations have been removed. It is required to migrate to v0.50 prior to upgrading to v0.51 for not missing any module migrations.

+ ```go
// InitGenesis performs genesis initialization for the module.
func (am AppModule) InitGenesis(ctx context.Context, data json.RawMessage) error {
}

// ExportGenesis returns the exported genesis state as raw bytes for the module.
func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) {
}
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 139-141: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

Most of Cosmos SDK modules have migrated to [collections](https://docs.cosmos.network/main/build/packages/collections).
Many functions have been removed due to this changes as the API can be smaller thanks to collections.
For modules that have migrated, verify you are checking against `collections.ErrNotFound` when applicable.

+ ```go
import authkeeper "cosmossdk.io/x/auth/keeper" 
...
err := authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper)
if err != nil {
	return nil, err
}
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 142-145: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

Accounts's AccountNumber will be used as a global account number tracking replacing Auth legacy AccountNumber. Must set accounts's AccountNumber with auth's AccountNumber value in upgrade handler. This is done through auth keeper MigrateAccountNumber function.

+ ```go
import authkeeper "cosmossdk.io/x/auth/keeper" 
...
err := authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper)
if err != nil {
	return nil, err
}
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


Line range hint 146-148: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.

Auth was spun out into its own `go.mod`. To import it use `cosmossdk.io/x/auth`

+ ```go
import authkeeper "cosmossdk.io/x/auth/keeper" 
...
err := authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper)
if err != nil {
	return nil, err
}
+ ```
Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 79ac6b4 and 38854b6.

Files selected for processing (1)
  • UPGRADING.md (1 hunks)
Additional context used
Path-based instructions (1)
UPGRADING.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Markdownlint
UPGRADING.md

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

Additional comments not posted (1)
UPGRADING.md (1)

Line range hint 149-151: Surround fenced code blocks with blank lines.

Ensure the fenced code blocks are surrounded by blank lines for better readability.
[n

Tools
LanguageTool

[typographical] ~43-~43: It appears that a comma is missing.
Context: ...AppOptions...) ``` ### SimApp In this section we describe the changes made in Cosmos ...

(DURING_THAT_TIME_COMMA)

Markdownlint

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 38854b6 and 99a166b.

Files selected for processing (1)
  • simapp/app.go (2 hunks)
Additional context used
Path-based instructions (1)
simapp/app.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (1)
simapp/app.go (1)

250-251: LGTM! But verify the function usage in the codebase.

The code change to include gas calculations for nested messages related to MsgSubmitProposal is approved.

However, ensure that all function calls to baseapp.SetIncludeNestedMsgsGas match the new usage.

Verification successful

Verification successful!

All function calls to baseapp.SetIncludeNestedMsgsGas match the new usage pattern.

  • simapp/app.go: []sdk.Msg{&govv1.MsgSubmitProposal{}}
  • baseapp/abci_test.go: []sdk.Msg{&baseapptestutil.MsgNestedMessages{}}
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `baseapp.SetIncludeNestedMsgsGas` match the new usage.

# Test: Search for the function usage. Expect: Only occurrences of the new usage.
rg --type go -A 5 $'baseapp.SetIncludeNestedMsgsGas'

Length of output: 800

@JulianToledano JulianToledano added this pull request to the merge queue Jul 26, 2024
Merged via the queue into main with commit fe8474e Jul 26, 2024
72 of 74 checks passed
@JulianToledano JulianToledano deleted the julian/nested-messages-simulation branch July 26, 2024 10:57
alpe added a commit that referenced this pull request Jul 26, 2024
* main:
  feat(log): remove core dependency and update core interface to be dependency free (#21045)
  chore: fix some comments (#21085)
  feat: simulate nested messages (#20291)
  chore(network): remove `DefaultConfigWithAppConfigWithQueryGasLimit` (#21055)
  fix(runtime): remove `appv1alpha1.Config` from runtime (#21042)
  feat(simapp/v2): Add store server to testnet init cmd (#21076)
  chore(indexer/postgres): update to changes on main (#21077)
  feat(schema/appdata): async listener mux'ing (#20879)
  ci: Use large box for 052 branch sims on CI (#21067)
  chore(all): replace all `fmt.Errorf` without paramters with `errors.New` (#21068)
@julienrbrt
Copy link
Member

Are we feature frozen? Is this expected this won't be in v0.52?

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

Successfully merging this pull request may close these issues.

[Feature]: simulate nested messages
6 participants