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

Add API for getting ministaker address (per challenge, per level) #508

Merged
merged 7 commits into from
Jan 4, 2024

Conversation

amsanghi
Copy link
Contributor

@amsanghi amsanghi commented Dec 19, 2023

@amsanghi amsanghi changed the base branch from main to honest_party_playing_subchallenge December 19, 2023 13:36
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 150 lines in your changes are missing coverage. Please review.

Comparison is base (781682d) 61.14% compared to head (3f963f8) 60.22%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
- Coverage   61.14%   60.22%   -0.93%     
==========================================
  Files          40       43       +3     
  Lines        6048     6436     +388     
==========================================
+ Hits         3698     3876     +178     
- Misses       2350     2511     +161     
- Partials        0       49      +49     

@amsanghi amsanghi requested a review from rauljordan December 19, 2023 13:55
@amsanghi amsanghi marked this pull request as ready for review December 19, 2023 13:55
Base automatically changed from honest_party_playing_subchallenge to main December 19, 2023 15:08
api/edges.go Outdated Show resolved Hide resolved
}
}
stakeInfoMap[edge.AssertionHash][edge.Type].StakerAddresses = append(stakeInfoMap[edge.AssertionHash][edge.Type].StakerAddresses, edge.MiniStaker)
stakeInfoMap[edge.AssertionHash][edge.Type].TotalMinistakes++
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be more useful if TotalMinistakes is the total staked amount rather than how many mini stakes are there, given the amount of mini stakes may vary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree on this, but currently we only have a fixed stake amount (the implementation for variable staking is not in place at the moment i think).
Since we have a fixed stake right now, adding that information does not seems that helpful (also since the stake amount is constant, its not attached to the edge proto, so fetching it is non trivial, but with the variable staking implementation that information will be attached to the edge and will be easier to fetch), will update the code once the variable staking is in place.

Also changing TotalMinistakes to NumberOfMinistakes to avoid confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed here that number of ministakes is helpful because it helps us gauge if there is a sybil more clearly

@amsanghi amsanghi merged commit 7b16331 into main Jan 4, 2024
7 of 8 checks passed
@amsanghi amsanghi deleted the ministakes branch January 4, 2024 12:21
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.

3 participants