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(launch): default chain account balances #936

Merged
merged 19 commits into from
Sep 1, 2022

Conversation

aljo242
Copy link

@aljo242 aljo242 commented Aug 26, 2022

Closes #927

What does this PR do?

Adds the DefaultAccountBalance field to Chain and MsgCreateChain. If this field is empty (sdk.NewCoins()). the behavior is unchanged. However, if it is populated, the balances of accounts that are created during keeper.ApplyRequest() are set to chain.DefaultAccountBalance.

How to test?

ignite chain serve --skip-proto -r
spnd tx profile create-coordinator --from alice   
spnd tx launch create-chain chain-1 "" "" --from alice --default-account-balance 1000test
spnd tx launch request-add-account 1 20000othercoins --from alice     
spnd q  launch list-genesis-account 1                             

@aljo242 aljo242 self-assigned this Aug 26, 2022
@aljo242
Copy link
Author

aljo242 commented Aug 26, 2022

I regenerated the proto files using make proto-gen and make proto-format so many of them were modified.

@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #936 (3d631b3) into develop (ae4ee27) will increase coverage by 0.02%.
The diff coverage is 18.70%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #936      +/-   ##
===========================================
+ Coverage    10.75%   10.77%   +0.02%     
===========================================
  Files          280      280              
  Lines        66957    67091     +134     
===========================================
+ Hits          7199     7227      +28     
- Misses       59595    59702     +107     
+ Partials       163      162       -1     
Impacted Files Coverage Δ
pkg/types/monitoring.pb.go 0.79% <ø> (ø)
x/campaign/types/events.pb.go 0.55% <ø> (ø)
x/campaign/types/mainnet_account.pb.go 0.75% <ø> (ø)
x/campaign/types/params.pb.go 0.88% <ø> (ø)
x/campaign/types/query.pb.go 0.66% <ø> (ø)
x/campaign/types/query.pb.gw.go 0.00% <ø> (ø)
x/launch/types/chain.pb.go 1.53% <0.00%> (-0.07%) ⬇️
x/launch/types/events.pb.go 0.49% <ø> (ø)
x/launch/types/genesis_account.pb.go 0.98% <ø> (ø)
x/launch/types/genesis_validator.pb.go 0.56% <ø> (ø)
... and 28 more

@lumtis
Copy link
Contributor

lumtis commented Aug 28, 2022

What's your reasoning to consider it should be named DefaultAccountBalance?

To me, it's a balance imposed by the coordinator. Default would mean this is the value set if the account doesn't provide a specific amount.

But it can be argued as well we can name it to "default" because this the balance that will be defined by default or accounts at first before the chain is launched and coins are moving

Copy link
Contributor

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Looks great

proto/launch/tx.proto Outdated Show resolved Hide resolved
@aljo242
Copy link
Author

aljo242 commented Aug 29, 2022

What's your reasoning to consider it should be named DefaultAccountBalance?

To me, it's a balance imposed by the coordinator. Default would mean this is the value set if the account doesn't provide a specific amount.

But it can be argued as well we can name it to "default" because this the balance that will be defined by default or accounts at first before the chain is launched and coins are moving

Yeah, I agree default doesn't make as much sense as I originally thought. I'm going to change it back to AccountBalance

x/campaign/keeper/keeper.go Outdated Show resolved Hide resolved
x/launch/client/cli/tx_create_chain.go Outdated Show resolved Hide resolved
x/launch/client/cli/tx_create_chain.go Outdated Show resolved Hide resolved
@lumtis
Copy link
Contributor

lumtis commented Aug 31, 2022

Hey @aljo242 is it ready for reviews?

I also wanted to add that we should add a test case in TestApplyRequest for the particular case the balance is defined

But I realize the current structure of TestApplyRequest is hard to maintain and should be improved first #944

@aljo242 aljo242 requested a review from lumtis September 1, 2022 12:50
@lumtis lumtis merged commit 1574562 into develop Sep 1, 2022
@lumtis lumtis deleted the feat/default-chain-account-balances branch September 1, 2022 13:10
@lumtis lumtis mentioned this pull request Sep 6, 2022
5 tasks
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.

Chain structure: add the ability to impose the account balance to be requested
2 participants