fix: Modify app path method to simulate for ABCI query

[jinoosss]

Descriptions

To simulate transactions, utilize the .app/simulate method for ABCI Query.

Changes

1. change the path of ABCI Query's .app to the result data storage location.

• You can receive the query result data as RequestQuery.ResponseData.Data instead of RequestQuery.Value.
• Provide it in a common form with other ABCI Queries.

2. remove the gas-consume logic of mocking signature data that is executed when simulating transactions (/tm2/pkg/sdk/auth/ante.go#L231-L237)

• We will get the correct value when simulating a real transaction.
• We want transactions to run without signatures, but we already have checks in place to see if a signature exists. (tm2/pkg/sdk/auth/ante.go#L104-L106)

Example

[Request Simulate]

curl --location 'http://localhost:26657' \
--header 'Content-Type: application/json' \
--data '{
  "id": 1,
  "jsonrpc": "2.0",
  "method": "abci_query",
  "params": [
    ".app/simulate",
    "CnMKDS9iYW5rLk1zZ1NlbmQSYgooZzFqZzhtdHV0dTlraGhmd2M0bnhtdWhjcGZ0ZjBwYWpkaGZ2c3FmNRIoZzFmZnp4aGE1N2RoMHFndjltYTV2MzkzdXIwemV4ZnZwNmxzanBhZRoMNTAwMDAwMHVnbm90Eg4IgIl6EggzMDB1Z25vdBp+CjoKEy90bS5QdWJLZXlTZWNwMjU2azESIwohA+FhNtsXHjLfSJk1lB8FbiL4mGPjc50Kt81J7EKDnJ2yEkCrIOTBt7YcDGcQ6Ohfv1r3nftAPaTATAtPfYD5zLQf7WDf1KPvWARe//CANtLLtIzcPVl7P/HnHxmfCYEwfGogIgUxMjMxMw==", 
    "0", 
    false
  ]
}'

[Response]

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "response": {
      "ResponseBase": {
        "Error": null,
        "Data": "eyJFcnJvciI6bnVsbCwiRGF0YSI6IiIsIkV2ZW50cyI6W10sIkxvZyI6Im1zZzowLHN1Y2Nlc3M6dHJ1ZSxsb2c6LGV2ZW50czpbXSIsIkluZm8iOiIiLCJHYXNXYW50ZWQiOjEwMDAwMDAsIkdhc1VzZWQiOjQ0NjI5fQ==",
        "Events": null,
        "Log": "",
        "Info": ""
      },
      "Key": null,
      "Value": null,
      "Proof": null,
      "Height": "0"
    }
  }
}

Related Issue

• [chain] Gas Estimations #1826

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tm2/pkg/sdk/baseapp.go	57.14%	1 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[zivkovicmilos]

@jinoosss

I'm not sure if this part is needed anymore in ProcessPubKey:

	if simulate {
		// In simulate mode the transaction comes with no signatures, thus if the
		// account's pubkey is nil, both signature verification and gasKVStore.Set()
		// shall consume the largest amount, i.e. it takes more gas to verify
		// secp256k1 keys than ed25519 ones.
		if pubKey == nil {
			return simSecp256k1Pubkey, sdk.Result{}
		}

		return pubKey, sdk.Result{}
	}

Can you please check? 🙏

[zivkovicmilos]

@jinoosss

I've dug into this a bit, and I believe we should stick to using ResponseQuery.Value to transfer the response of the query.

It's seems to be a standard practice in Cosmos, that query results are stored within the response query Value. (It's the value part of the key query)

ResponseBase.Data is typically used for additional metadata or any auxiliary information that may accompany the primary response (in ResponseQuery.Value).

I'm sorry for the change-up, we should switch to using ResponseQuery.Value as it was 🙏

[zivkovicmilos]

@jinoosss
Did you get a chance to check this? 👀
#3207 (comment)

[jinoosss]

@zivkovicmilos ,
Thank you for checking it out.

Let's check the following
#3207 (comment)

[jinoosss]

@jinoosss

I'm not sure if this part is needed anymore in ProcessPubKey:

	if simulate {
		// In simulate mode the transaction comes with no signatures, thus if the
		// account's pubkey is nil, both signature verification and gasKVStore.Set()
		// shall consume the largest amount, i.e. it takes more gas to verify
		// secp256k1 keys than ed25519 ones.
		if pubKey == nil {
			return simSecp256k1Pubkey, sdk.Result{}
		}

		return pubKey, sdk.Result{}
	}

Can you please check? 🙏

I think it's fine to remove it.

The account coming in as an argument is generated from the transaction's signature.

The contents of the pubKey == nil condition is not executed because this logic is checked against the transaction before it is executed.

[Gno2D2]

I'm a bot that assists the Gno Core team in maintaining this repository. My role is to ensure that contributors understand and follow our guidelines, helping to streamline the development process.

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🟢 Maintainers must be able to edit this pull request
🟢 The pull request head branch must be up-to-date with its base

Manual Checks

• The pull request description provides enough details (checked by @zivkovicmilos)

Debug

Automated Checks

Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (jinoosss:fix/modify-app-path-method-to-utilize-simulator-for-abci-queries) is up to date with base (master): behind by 0 / ahead by 15

Manual Checks

The pull request description provides enough details

If

🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)

Can be checked by

• team core-contributors