-
Notifications
You must be signed in to change notification settings - Fork 370
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
Make the same hotfix proposal executable with unique salts #2357
Conversation
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.
Looking good! Left some comments, but approved already!
bfedb0b
to
640c70a
Compare
Codecov Report
@@ Coverage Diff @@
## master #2357 +/- ##
=======================================
Coverage 74.55% 74.55%
=======================================
Files 278 278
Lines 7777 7777
Branches 991 991
=======================================
Hits 5798 5798
Misses 1868 1868
Partials 111 111
Continue to review full report at Codecov.
|
@@ -655,6 +655,7 @@ contract Governance is | |||
* @param hash The abi encoded keccak256 hash of the hotfix transaction(s) to be whitelisted. | |||
*/ | |||
function approveHotfix(bytes32 hash) external { | |||
require(!hotfixes[hash].executed, "hotfix already executed"); |
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.
Make this a modifier?
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.
fixing in #2340
@@ -78,7 +79,9 @@ export const handler = async (argv: EthstatsArgv) => { | |||
console.error('\nPlease see examples in hotfix.ts and add transactions') | |||
process.exit(1) | |||
} | |||
const proposalHash = proposalToHash(kit, proposal) | |||
|
|||
const salt = randomBytes(32) |
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.
should this be logged in case something goes wrong?
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.
ditto
@@ -696,15 +699,17 @@ contract Governance is | |||
* @param destinations The destination addresses of the proposed transactions. | |||
* @param data The concatenated data to be included in the proposed transactions. | |||
* @param dataLengths The lengths of each transaction's data. | |||
* @param salt Secret associated with hotfix which guarantees uniqueness of hash. |
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.
nit: Does this need to be secret?
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.
ditto
* master: (139 commits) Accelerate time based parameters (#2377) Fix circle not being started (#2380) Blockscout logs patch for json format (#2268) [Wallet] Fix slow activity feed tx confirmations (#2290) Aaronmgdr/about followup (#2376) Change "not syncing" to "not synced" (#2372) [Docs] Update Contributing guide with links to good first issues (#2375) Optimize Bundles (#2352) Update blockscout for alfajores and pilot envs (#1954) Make the same hotfix proposal executable with unique salts (#2357) Fix Contribution Guildelines in the docs (#2370) Add Snapshot for each page on website (#2313) Turn on Emit (#2369) Update to latest blockchain with select issuers fix (#2362) Add docs on how to generate sms retriever hash (#2356) Aaronmgdr/about below (#1194) Switch to using native pbkdf to fix hanging on older devices (#2354) Adding a command to the docker script allowing to stop validating (#2295) Update prettier to 1.19.1 to support TypeScript 3.7 (optional chaining, nullish coalescing, etc) (#2358) Fixes needed to make slashing work (#2346) ...
Description
Updates governance contract, contractkit and cli interfaces to accommodate the same hotfix proposal executing multiple times
Tested
Unit tests updated in protocol
Other changes
Adds unexecuted requirement to
whitelist
,approve
, andprepare
,Related issues