-
Notifications
You must be signed in to change notification settings - Fork 354
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
Minor governance module improvements plus adding veryfing_contract to Delegation #1214
Minor governance module improvements plus adding veryfing_contract to Delegation #1214
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1214 +/- ##
=======================================
Coverage 92.06% 92.06%
=======================================
Files 49 49
Lines 1398 1399 +1
=======================================
+ Hits 1287 1288 +1
Misses 111 111
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improvements look good! I left a few minor suggestions and a question
pub trait VotingUnitsTrait<TState> { | ||
/// Returns the number of voting units for a given account. For ERC20, this is typically the | ||
/// token balance. For ERC721, this is typically the number of tokens owned. | ||
/// | ||
/// WARNING: While any formula can be used as a measure of voting units, the internal vote | ||
/// accounting of the contract may be compromised if voting units are transferred in any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically breaking since we're changing its path, no? I'd probably add it to the changelog since it's public. WDYT?
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Andrew regarding mentioning in the changelog that VotingUnitsTrait
has been moved. Apart from that, LGTM!
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
No description provided.