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 Simulation Bug #4714

Merged
merged 13 commits into from
Jul 12, 2019
Merged

Fix Simulation Bug #4714

merged 13 commits into from
Jul 12, 2019

Conversation

colin-axner
Copy link
Contributor

The simulation will currently use the ModuleAccount addresses for standard operations such as delegate, send, etc.

{"entry_kind":"msg","height":15,"order":365,"operation":{"route":"staking","name":"delegate","comment":"","ok":true,"msg":{"type":"cosmos-sdk/MsgDelegate","value":{"amount":{"amount":"62403758388","denom":"uatom"},"delegator_address":"cosmos1jv65s3grqf6v6jl3dp4t6c9t9rk99cd88lyufl","validator_address":"cosmosvaloper1asazdfn60g4nakccc9leh37du0946l5fnnluy6"}}}}
DISTRIBUTION ACCOUNT ADDRESS: cosmos1jv65s3grqf6v6jl3dp4t6c9t9rk99cd88lyufl

To fix this the module account addresses were added to a map in simapp. Then random accounts are checked against this map to ensure the address does not match. Also, after pulling from genesis, accounts who have a module account address are removed from the accounts used for random activity.

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #4714 into supply-genesis will decrease coverage by <.01%.
The diff coverage is 7.07%.

@@                Coverage Diff                 @@
##           supply-genesis    #4714      +/-   ##
==================================================
- Coverage           54.12%   54.12%   -0.01%     
==================================================
  Files                 272      273       +1     
  Lines               17386    17399      +13     
==================================================
+ Hits                 9411     9417       +6     
- Misses               7290     7297       +7     
  Partials              685      685

x/simulation/account.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks for finding this bug @colin-axner @AdityaSripal !

I think that this logic can be simplified if we avoid returning a blacklisted account on the RandomAcc function from x/simulation/account.go as that's ultimately where the account is selected from the array.

Also, on GenGenesisAccounts we create the genesis accounts (vesting or base accounts) based on randomness but we should also create the genesis accounts from the blacklisted module accounts.

simapp/app.go Show resolved Hide resolved
simapp/app.go Show resolved Hide resolved
x/simulation/account.go Outdated Show resolved Hide resolved
x/simulation/simulate.go Show resolved Hide resolved
x/simulation/simulate.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

ACK - Fede's comments should be addressed first

@fedekunze
Copy link
Collaborator

also, let's rebase this to the supply-genesis branch

@colin-axner colin-axner changed the base branch from master to supply-genesis July 12, 2019 17:25
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK

simapp/app.go Show resolved Hide resolved
@alessio alessio merged commit 32d2be2 into supply-genesis Jul 12, 2019
@alessio alessio deleted the colin/fix-sim-bug branch July 12, 2019 17:38
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.

7 participants