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

[framework] governance multi-step proposal #5445

Merged
merged 1 commit into from
Dec 7, 2022
Merged

[framework] governance multi-step proposal #5445

merged 1 commit into from
Dec 7, 2022

Conversation

0xchloe
Copy link
Contributor

@0xchloe 0xchloe commented Nov 4, 2022

Description

Implementation of this AIP: aptos-foundation/AIPs#3.

TLDR: we're supporting multi-step proposals in this PR. We do so by updating proposal.execution_hash and ApprovedExecutionHashes on-chain to the next_execution_hash.

Summary of changes included in this PR:

aptos_governance module:
Added:

  • aptos_governance::create_proposal_v2(): this is similar to the existing aptos_governance::create_proposal, but here we call the newly added voting::create_proposal_v2 to create an Aptos governance proposal.

  • aptos_governance::resolve_multi_step_proposal(): similar to the existing aptos_governance::resolve(), but here we replace the current execution hash in ApprovedExecutionHashes with next_execution_hash, and call voting::resolve_proposal_v2 to resolve the proposal.

Modified:

  • aptos_governance::add_approved_script_hash(): previously, we immediately return if the ApprovedExecutionHashes map already contains the given proposal_id. In the updated code, if the ApprovedExecutionHashes map already contains the given proposal_id, we will replace the existing value in ApprovedExecutionHashes with the updated execution hash.

  • aptos_governance::create_proposal(): redirected this function to call the newly added aptos_governance::create_proposal_v2(), so that we'll have metadata information about if this proposal is single-step or multi-step.

voting module:
Added:

  • Two new constants: IS_MULTI_STEP_PROPOSAL_KEY and IS_MULTI_STEP_PROPOSAL_IN_EXECUTION_KEY. They will be stored in the proposal.metadata map upon creation of a proposal.

  • voting:: create_proposal_v2(): the flow of this function is similar to the existing voting:: create_proposal() function except that we are also adding metadata about this proposal in the proposal.metadata map.

    • IS_MULTI_STEP_PROPOSAL_KEY: true for multi-step proposals, false for single-step proposals;
    • IS_MULTI_STEP_PROPOSAL_IN_EXECUTION_KEY only exists in multi-step proposals. Its value is by default false.
      We update it to true when we start resolving the multi-step proposal. This flag is used to disable further voting when a multi-step proposal is already in execution.
  • voting:: is_proposal_resolvable(): this includes all common checks on if a proposal is resolvable regardless if the proposal is single-step or multi-step.

  • voting:: resolve_proposal_v2(): this function can resolve a single-step proposal or a multi-step proposal. The flow for a single-step proposal remains mostly unchanged. The flow for a multi-step proposal will update the proposal.execution_hash with the next_execution_hash on-chain, and check/update proposal.metadata values.

  • voting:: is_multi_step_proposal_in_execution(): this function returns true if the specified multi-step proposal is in execution.

Modified:

  • voting::vote(): added a check to abort when the specified proposal is multi-step and already in execution.

  • voting::resolve(): added a check to ensure that this function can only be called to resolve single-step proposals.

Test Plan

Unit tests in this PR, smoke + e2e tests to come in future PRs.


This change is Reviewable

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

ahh this is clever!

@0xchloe 0xchloe force-pushed the governance branch 7 times, most recently from 70ca1fd to 192338f Compare November 23, 2022 05:09
@0xchloe 0xchloe marked this pull request as ready for review November 23, 2022 05:09
@0xchloe 0xchloe changed the title draft for multi-step governance proposal [framework] governance multi-step proposal Nov 23, 2022
@0xchloe 0xchloe force-pushed the governance branch 3 times, most recently from 2f6e0fa to 9fe7465 Compare December 2, 2022 04:34
@0xchloe 0xchloe force-pushed the governance branch 2 times, most recently from 051c0e4 to 5c8b6cf Compare December 3, 2022 01:08
Copy link
Contributor

@movekevin movekevin left a comment

Choose a reason for hiding this comment

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

Just a few minor comments left. Otherwise it's good to go :)

@0xchloe 0xchloe force-pushed the governance branch 3 times, most recently from 77a33a8 to b46c395 Compare December 5, 2022 23:55
Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

Left a minor nit but the overall logic makes sense to me! Great work!

/// 1. RESOLVABLE_TIME_METADATA_KEY: this is uesed to record the resolvable time to ensure that resolution has to be done non-atomically.
/// 2. IS_MULTI_STEP_PROPOSAL_KEY: this is used to track if a proposal is single-step or multi-step.
/// 3. IS_MULTI_STEP_PROPOSAL_IN_EXECUTION_KEY: this attribute only exists for and applies to multi-step proposals. The value is used to
/// indicate if a multi-step proposal is in execution. If yes, we will disable further voting for this multi-step proposal.
metadata: SimpleMap<String, vector<u8>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great if we can implement new move features to avoid having this heterogeneous map...

Copy link
Contributor Author

@0xchloe 0xchloe Dec 7, 2022

Choose a reason for hiding this comment

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

ahh yeah, any ideas on how we can make this better? it does seem kind of hacky to hardcode all the keys in this map.

/// @param voting_forum_address The address of the forum where the proposals are stored.
/// @param proposal_id The proposal id.
public fun resolve<ProposalType: store>(
fun is_proposal_resolvable<ProposalType: store>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is a bad name because the function is actually mutating global states i.e: update is_multi_step_proposal_in_execution_value, but the name suggests it's a pure function. Would be great if you can find a new name and provide some comments there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great point - will update the name of this function and add more comments!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just updated! I kept the name of this function but added some comment and moved the is_multi_step_proposal_in_execution_value update logic to resolve_proposal_v2(), since this logic doesn't apply to single-step proposal (so it's not a common check, and doesn't make sense to have it in is_proposal_resolvable()).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

✅ Forge suite land_blocking success on c10bc4e477736be8548a80de8c061102e3cbaa2d

performance benchmark with full nodes : 6506 TPS, 6097 ms latency, 9700 ms p99 latency,(!) expired 380 out of 2778520 txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> c10bc4e477736be8548a80de8c061102e3cbaa2d

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> c10bc4e477736be8548a80de8c061102e3cbaa2d (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7510 TPS, 5137 ms latency, 6800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: c10bc4e477736be8548a80de8c061102e3cbaa2d
compatibility::simple-validator-upgrade::single-validator-upgrade : 4580 TPS, 8831 ms latency, 12200 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: c10bc4e477736be8548a80de8c061102e3cbaa2d
compatibility::simple-validator-upgrade::half-validator-upgrade : 4969 TPS, 7883 ms latency, 10100 ms p99 latency,no expired txns
4. upgrading second batch to new version: c10bc4e477736be8548a80de8c061102e3cbaa2d
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6658 TPS, 5761 ms latency, 10600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> c10bc4e477736be8548a80de8c061102e3cbaa2d passed
Test Ok

@0xchloe 0xchloe merged commit bbdfd32 into main Dec 7, 2022
@0xchloe 0xchloe deleted the governance branch December 7, 2022 04:14
areshand pushed a commit to areshand/aptos-core-1 that referenced this pull request Dec 18, 2022
Co-authored-by: chloeqjz <79347459+chloeqjz@users.noreply.github.com>
@Markuze Markuze mentioned this pull request Dec 26, 2022
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.

4 participants