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: add x/group simulations #251

Merged
merged 96 commits into from
May 11, 2021
Merged

feat: add x/group simulations #251

merged 96 commits into from
May 11, 2021

Conversation

aleem1314
Copy link
Contributor

@aleem1314 aleem1314 commented Feb 5, 2021

closes: #240

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #251 (ae4af63) into master (40e2a34) will decrease coverage by 0.34%.
The diff coverage is 21.73%.

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   61.74%   61.39%   -0.35%     
==========================================
  Files          48       50       +2     
  Lines        3100     3119      +19     
==========================================
+ Hits         1914     1915       +1     
- Misses        947      965      +18     
  Partials      239      239              

x/group/module/module.go Outdated Show resolved Hide resolved
x/group/module/module.go Outdated Show resolved Hide resolved
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.

LGTM

x/group/simulation/operations.go Outdated Show resolved Hide resolved
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

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

lgtm overall, nice one @aleem1314, thanks!

x/group/simulation/operations.go Outdated Show resolved Hide resolved
@blushi
Copy link
Member

blushi commented Apr 14, 2021

@anilcse could you have a final look at this when you have time so we can eventually merge it? Thanks.

Copy link
Contributor

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

implementation lgtm. left a few questions

sims.mk Outdated Show resolved Hide resolved
sims.mk Show resolved Hide resolved
x/group/module/module.go Outdated Show resolved Hide resolved
x/group/simulation/operations.go Outdated Show resolved Hide resolved
@aleem1314 aleem1314 requested a review from anilcse April 15, 2021 07:38
@blushi
Copy link
Member

blushi commented Apr 23, 2021

@aleem1314 could you resolve the conflicts so we can merge when you have time?

@blushi
Copy link
Member

blushi commented Apr 23, 2021

@anilcse are we good to merge this?

@aleem1314
Copy link
Contributor Author

@anilcse are we good to merge this?

Don't merge. One simulation job is failing.

@blushi
Copy link
Member

blushi commented May 5, 2021

@anilcse are we good to merge this?

Don't merge. One simulation job is failing.

It seems like it's due to group invariants being broken, could you have a look when you have time @aleem1314?

@aleem1314 aleem1314 changed the title x/group: add simulations feat: add x/group simulations May 5, 2021
@aleem1314
Copy link
Contributor Author

@anilcse can you take a final look?

@blushi
Copy link
Member

blushi commented May 6, 2021

@aleem1314 are we good to merge this?

@blushi blushi merged commit efcc268 into master May 11, 2021
@blushi blushi deleted the aleem/groups-simulations branch May 11, 2021 08:32
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.

[x/group] Add simulations
5 participants