Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add fixed fee for creating a new credit class (#351) #375

Merged
merged 4 commits into from
Jun 16, 2021
Merged

feat: Add fixed fee for creating a new credit class (#351) #375

merged 4 commits into from
Jun 16, 2021

Conversation

ruhatch
Copy link
Contributor

@ruhatch ruhatch commented Jun 8, 2021 •
edited by zube bot
Loading

This PR adds a fixed fee to the CreateClass method of x/ecocredit.

The fee is:

While reviewing this PR, please pay extra attention to:

  • The module structure: Is this idiomatic Cosmos SDK module layout?
  • The default genesis value: Is this a sensible initial value?

Closes #351

@ruhatch ruhatch changed the title [#351] Add fixed fee for creating a new credit class feat: Add fixed fee for creating a new credit class (#351) Jun 8, 2021
@ruhatch ruhatch force-pushed the ruhatch/credit-class-fee branch 5 times, most recently from a454fdf to 4f52a56 Compare June 9, 2021 11:18
@codecov
Copy link

codecov bot commented Jun 9, 2021 •
edited
Loading

Codecov Report

Merging #375 (fccafe7) into master (fc41f22) will increase coverage by 0.27%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   60.74%   61.01%   +0.27%     
==========================================
  Files          49       51       +2     
  Lines        3289     3317      +28     
==========================================
+ Hits         1998     2024      +26     
- Misses       1028     1029       +1     
- Partials      263      264       +1
Flag Coverage Δ
experimental-codecov 61.01% <93.54%> (+0.27%) ⬆️
stable-codecov 61.01% <93.54%> (+0.27%) ⬆️

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

@ruhatch ruhatch marked this pull request as ready for review June 9, 2021 11:19
@ruhatch ruhatch self-assigned this Jun 9, 2021
@ruhatch ruhatch mentioned this pull request Jun 10, 2021
Closed
@blushi blushi assigned blushi and unassigned ruhatch Jun 10, 2021
blushi
blushi previously requested changes Jun 11, 2021
View reviewed changes
Copy link
Member

@blushi blushi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Implementation looks good overall but I would rework the module wiring.

Could you resolve the conflicts?

third_party/proto/cosmos/base/v1beta1/coin.proto Show resolved Hide resolved
x/ecocredit/server/msg_server.go Outdated Show resolved Hide resolved
x/ecocredit/types.go Outdated Show resolved Hide resolved
x/ecocredit/keeper/keeper.go Outdated Show resolved Hide resolved
x/ecocredit/module/module.go Outdated Show resolved Hide resolved
x/ecocredit/module/module.go Outdated Show resolved Hide resolved
@ruhatch ruhatch force-pushed the ruhatch/credit-class-fee branch 3 times, most recently from 1bc7219 to 15d827f Compare June 14, 2021 12:38
@ruhatch ruhatch requested a review from blushi June 14, 2021 14:02
@aaronc
Copy link
Member

aaronc commented Jun 14, 2021

Btw no need to rebase and force push @ruhatch. Since we're squashing everything you can just use merge commits

@ruhatch
Copy link
Contributor Author

ruhatch commented Jun 14, 2021

Btw no need to rebase and force push @ruhatch. Since we're squashing everything you can just use merge commits

Nice, thanks for the tip!

likhita-809
likhita-809 reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@likhita-809 likhita-809 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good, just need few changes

x/ecocredit/server/server_test.go Outdated Show resolved Hide resolved
x/ecocredit/server/server_test.go Outdated Show resolved Hide resolved
ruhatch and others added 2 commits June 15, 2021 14:08
@ruhatch @likhita-809
Update x/ecocredit/server/server_test.go
6bbb05d 
Co-authored-by: likhita-809 <78951027+likhita-809@users.noreply.github.com>
@ruhatch @likhita-809
Update x/ecocredit/server/server_test.go
e982646 
Co-authored-by: likhita-809 <78951027+likhita-809@users.noreply.github.com>
@ruhatch ruhatch requested a review from likhita-809 June 15, 2021 13:09
@ruhatch ruhatch dismissed blushi’s stale review June 15, 2021 13:52

Changes implemented

@ruhatch ruhatch enabled auto-merge (squash) June 15, 2021 15:10
blushi
blushi requested changes Jun 16, 2021
View reviewed changes
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

I've tested it with the CLI and tx is failing with (coming from ABCI, recovering from a panic):
panic message redacted to hide potentially sensitive system info: panic
The tx is working fine on master. Can you reproduce it?

x/ecocredit/server/fee.go Outdated Show resolved Hide resolved
x/ecocredit/server/fee.go Outdated Show resolved Hide resolved
x/ecocredit/server/genesis.go Show resolved Hide resolved
x/ecocredit/server/msg_server.go Outdated Show resolved Hide resolved
@ruhatch
Copy link
Contributor Author

ruhatch commented Jun 16, 2021

I've tested it with the CLI and tx is failing with (coming from ABCI, recovering from a panic):
panic message redacted to hide potentially sensitive system info: panic
The tx is working fine on master. Can you reproduce it?

@blushi did you init a new chain or use an existing one? It might be panicking because it can't find the genesis parameters?

@blushi
Copy link
Member

blushi commented Jun 16, 2021

I've tested it with the CLI and tx is failing with (coming from ABCI, recovering from a panic):
panic message redacted to hide potentially sensitive system info: panic
The tx is working fine on master. Can you reproduce it?

@blushi did you init a new chain or use an existing one? It might be panicking because it can't find the genesis parameters?

Good point! That was indeed with an existing chain, I'll try out again with a new one.

blushi
blushi approved these changes Jun 16, 2021
View reviewed changes
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

tACK, nice work @ruhatch!

@ruhatch ruhatch merged commit f7b6d2f into master Jun 16, 2021
@ruhatch ruhatch deleted the ruhatch/credit-class-fee branch June 16, 2021 11:45
@ryanchristo ryanchristo mentioned this pull request Mar 31, 2022
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronc aaronc aaronc left review comments

@blushi blushi blushi approved these changes

@robert-zaremba robert-zaremba Awaiting requested review from robert-zaremba

@anilcse anilcse Awaiting requested review from anilcse

@aleem1314 aleem1314 Awaiting requested review from aleem1314

@amaury1093 amaury1093 Awaiting requested review from amaury1093

@atheeshp atheeshp Awaiting requested review from atheeshp

@clevinson clevinson Awaiting requested review from clevinson

@cyberbono3 cyberbono3 Awaiting requested review from cyberbono3

@ryanchristo ryanchristo Awaiting requested review from ryanchristo

@technicallyty technicallyty Awaiting requested review from technicallyty

@likhita-809 likhita-809 Awaiting requested review from likhita-809

Assignees

@blushi blushi

@likhita-809 likhita-809

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add fixed fee for creating a new credit class
4 participants
@ruhatch @aaronc @blushi @likhita-809