Skip to content

feat!: Add gas limits to queries#16239

Merged
julienrbrt merged 14 commits into
mainfrom
dev/add_query_gas_limit
Aug 8, 2023
Merged

feat!: Add gas limits to queries#16239
julienrbrt merged 14 commits into
mainfrom
dev/add_query_gas_limit

Conversation

@ValarDragon
Copy link
Copy Markdown
Contributor

@ValarDragon ValarDragon commented May 20, 2023 •
edited
Loading

Description

Closes #16237

Added a baseapp test. Is there anywhere where we test e2e that the new config value is read correctly and passed into baseapp? I didn't see such a test anywhere

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
  • added ! to 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
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • 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 ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@ValarDragon ValarDragon changed the title feature: Add gas limits to queries feat!: Add gas limits to queries May 20, 2023
@ValarDragon ValarDragon marked this pull request as ready for review May 20, 2023 18:09
@ValarDragon ValarDragon requested a review from a team May 20, 2023 18:09
@ghost ghost requested review from a team, JeancarloBarrios and samricotta and removed request for a team May 20, 2023 18:09
@ValarDragon
Copy link
Copy Markdown
Contributor Author

ValarDragon commented May 20, 2023 •
edited
Loading

Is there a clean way to do a go test, that shows what the error message will be if you do a grpc query that exceeds the gas limit?

@alexanderbez
Copy link
Copy Markdown
Contributor

alexanderbez commented May 20, 2023

We've essentially wrapped up #15519 -- we just need review. I would personally like to see that merged prior to any major baseapp changes, but I'm not opposed to getting this in.

julienrbrt
julienrbrt reviewed Jun 2, 2023
View reviewed changes
Comment thread server/config/toml.go
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 9, 2023

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 Jul 9, 2023
alexanderbez
alexanderbez approved these changes Jul 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK. The approach looks correct to me. I think @julienrbrt had a comment or two. Mind addressing that quickly @ValarDragon?

Comment thread baseapp/baseapp.go Outdated
@github-actions github-actions Bot removed the Stale label Jul 11, 2023
odeke-em
odeke-em approved these changes Aug 6, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @ValarDragon!

@tac0turtle @alexanderbez
Update baseapp/baseapp.go
056ea69 
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
tac0turtle
tac0turtle approved these changes Aug 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

overall looks good, sorry for taking a while to get this in.

julienrbrt
julienrbrt reviewed Aug 7, 2023
View reviewed changes
Comment thread tools/confix/data/v0.50-app.toml Outdated
@tac0turtle tac0turtle requested a review from julienrbrt August 8, 2023 14:43
julienrbrt
julienrbrt approved these changes Aug 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

utACK.

Comment thread CHANGELOG.md Outdated
Comment thread baseapp/baseapp_test.go Outdated
@julienrbrt julienrbrt enabled auto-merge August 8, 2023 20:18
@julienrbrt julienrbrt added this pull request to the merge queue Aug 8, 2023
Merged via the queue into main with commit dea5b97 Aug 8, 2023
@julienrbrt julienrbrt deleted the dev/add_query_gas_limit branch August 8, 2023 20:32
mergify Bot pushed a commit that referenced this pull request Aug 8, 2023
@ValarDragon @tac0turtle
@alexanderbez @julienrbrt @mergify
feat!: Add gas limits to queries (#16239)
044e108 
Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
(cherry picked from commit dea5b97)
@mergify mergify Bot mentioned this pull request Aug 8, 2023
Merged
tac0turtle pushed a commit that referenced this pull request Aug 9, 2023
@mergify @ValarDragon
feat!: Add gas limits to queries (backport #16239) (#17324)
bee95b1 
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@chipshort chipshort mentioned this pull request Oct 3, 2024
Open
@faddat faddat mentioned this pull request Nov 8, 2024
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JeancarloBarrios JeancarloBarrios Awaiting requested review from JeancarloBarrios

@samricotta samricotta Awaiting requested review from samricotta

4 more reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@odeke-em odeke-em odeke-em approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@julienrbrt julienrbrt julienrbrt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alexanderbez alexanderbez

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature]: Add GasMetering for queries

5 participants

@ValarDragon @alexanderbez @odeke-em @tac0turtle @julienrbrt