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

fix: data and ecocredit gas consumption #934

Merged
merged 6 commits into from
Mar 25, 2022
Merged

Conversation

ryanchristo
Copy link
Member

@ryanchristo ryanchristo commented Mar 24, 2022

Description

Closes: #185

This pull request fixes and updates gas consumption for the data module and ecocredit module. Ensures gas consumption for all iterations as well as hash verification in the data module. Also updates gas consumption descriptions.


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)

@ryanchristo ryanchristo marked this pull request as ready for review March 24, 2022 19:13
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #934 (d0f2288) into master (1df8b21) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head d0f2288 differs from pull request most recent head 3b68be3. Consider uploading reports for the commit 3b68be3 to get more accurate results

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
+ Coverage   72.34%   72.44%   +0.10%     
==========================================
  Files         194      194              
  Lines       22892    22888       -4     
==========================================
+ Hits        16562    16582      +20     
+ Misses       5088     5051      -37     
- Partials     1242     1255      +13     
Flag Coverage Δ
experimental-codecov 72.26% <100.00%> (+<0.01%) ⬆️
stable-codecov 65.58% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

nice improvement.


// TODO: Revisit this once we have proper gas fee framework.
// Tracking issue https://github.com/cosmos/cosmos-sdk/discussions/9072
const GasCostPerIteration = uint64(10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

seams we are copying this value all over different places. Can we use it form a single place?
I know we are using a different modules, but maybe we can create some params module for that. (not necessary in this task). Something to discuss, not necessary do in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea. Although if we want to set module specific values, we might want to keep them within each module, same way we do module specific errors. Currently we use 10 for all iterations but we might want to have some iterations charge more gas than others. I thought about making those changes here but figured I would start with making sure we are at least charging some gas for each iteration.

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

lgtm - some files have imports ungrouped

@@ -2,6 +2,7 @@ package core

import (
"context"
"github.com/regen-network/regen-ledger/x/ecocredit"
Copy link
Contributor

Choose a reason for hiding this comment

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

some of these imports didn't appear to join the existing /regen-ledger/ group

Copy link
Member Author

Choose a reason for hiding this comment

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

I just updated my golang preferences too. Thanks for sharing that over at the watercooler. 🙂

@ryanchristo ryanchristo enabled auto-merge (squash) March 25, 2022 20:44
@ryanchristo ryanchristo merged commit 70f0264 into master Mar 25, 2022
@ryanchristo ryanchristo deleted the ryan/185-gas-consumption branch March 25, 2022 20:51
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.

Add max data size consts for x/data and x/ecocredit + additional gas for hash & storage
3 participants