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.

Sign up for GitHub

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

Jump to bottom

[Stake] Allow to set the beneficiary for operator #10455

Merged
merged 2 commits into from
Oct 24, 2023
Merged

[Stake] Allow to set the beneficiary for operator #10455

merged 2 commits into from
Oct 24, 2023

Conversation

junkil-park
Copy link
Contributor

@junkil-park junkil-park commented Oct 10, 2023 •
edited
Loading

Description

This PR allows the operator to set the beneficiary address. This PR implements the AIP-51: aptos-foundation/AIPs#252

Test Plan

aptos move test

@junkil-park junkil-park force-pushed the jpark/change-beneficiary branch from b98bf94 to 96c3383 Compare October 15, 2023 08:38
@junkil-park junkil-park marked this pull request as ready for review October 15, 2023 08:54
@junkil-park junkil-park requested a review from movekevin October 15, 2023 08:55
@junkil-park junkil-park force-pushed the jpark/change-beneficiary branch 3 times, most recently from caf7eb7 to e32e989 Compare October 16, 2023 16:45
@junkil-park junkil-park force-pushed the jpark/change-beneficiary branch 2 times, most recently from 3314d1a to d93980f Compare October 20, 2023 07:00
@junkil-park junkil-park requested review from alexfilip2, michelle-aptos and lightmark and removed request for alexfilip2 October 20, 2023 07:01
@junkil-park junkil-park force-pushed the jpark/change-beneficiary branch 2 times, most recently from dd94a12 to 8d850b5 Compare October 20, 2023 08:35
lightmark
lightmark approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@lightmark lightmark left a comment

Choose a reason for hiding this comment

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

Not quite understand the design of putting beneficiary under operators account.
The events naming may need to be fixed.
otherwise looks good. unblock the PR

aptos-move/framework/aptos-framework/sources/delegation_pool.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/delegation_pool.move Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/delegation_pool.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/delegation_pool.move Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/delegation_pool.move Show resolved Hide resolved
@junkil-park junkil-park force-pushed the jpark/change-beneficiary branch 4 times, most recently from 6abbcc3 to 5c1a440 Compare October 24, 2023 00:59
@junkil-park junkil-park force-pushed the jpark/change-beneficiary branch from 5c1a440 to bab7d34 Compare October 24, 2023 18:41
@junkil-park junkil-park enabled auto-merge (squash) October 24, 2023 18:45
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.7.2 ==> bab7d34c8b1fc27b7718e8416aa9a39c0430096c

Compatibility test results for aptos-node-v1.7.2 ==> bab7d34c8b1fc27b7718e8416aa9a39c0430096c (PR)
1. Check liveness of validators at old version: aptos-node-v1.7.2
compatibility::simple-validator-upgrade::liveness-check : committed: 5702 txn/s, latency: 5830 ms, (p50: 5100 ms, p90: 10200 ms, p99: 12600 ms), latency samples: 228080
2. Upgrading first Validator to new version: bab7d34c8b1fc27b7718e8416aa9a39c0430096c
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1818 txn/s, latency: 15917 ms, (p50: 18600 ms, p90: 22300 ms, p99: 22500 ms), latency samples: 92740
3. Upgrading rest of first batch to new version: bab7d34c8b1fc27b7718e8416aa9a39c0430096c
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1842 txn/s, latency: 15658 ms, (p50: 18600 ms, p90: 22300 ms, p99: 22500 ms), latency samples: 92100
4. upgrading second batch to new version: bab7d34c8b1fc27b7718e8416aa9a39c0430096c
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3617 txn/s, latency: 8819 ms, (p50: 9600 ms, p90: 12500 ms, p99: 12700 ms), latency samples: 144700
5. check swarm health
Compatibility test for aptos-node-v1.7.2 ==> bab7d34c8b1fc27b7718e8416aa9a39c0430096c passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on bab7d34c8b1fc27b7718e8416aa9a39c0430096c

two traffics test: inner traffic : committed: 7845 txn/s, latency: 4985 ms, (p50: 4800 ms, p90: 6000 ms, p99: 11700 ms), latency samples: 3420700
two traffics test : committed: 100 txn/s, latency: 2335 ms, (p50: 2200 ms, p90: 2800 ms, p99: 7600 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.206, avg: 0.199", "QsPosToProposal: max: 0.170, avg: 0.153", "ConsensusProposalToOrdered: max: 0.662, avg: 0.618", "ConsensusOrderedToCommit: max: 0.560, avg: 0.537", "ConsensusProposalToCommit: max: 1.192, avg: 1.155"]
Max round gap was 1 [limit 4] at version 980972. Max no progress secs was 4.461443 [limit 10] at version 3435774.
Test Ok

@junkil-park junkil-park merged commit 61d80ec into main Oct 24, 2023
70 of 84 checks passed
@junkil-park junkil-park deleted the jpark/change-beneficiary branch October 24, 2023 19:24
@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.7.2 ==> bab7d34c8b1fc27b7718e8416aa9a39c0430096c

Compatibility test results for aptos-node-v1.7.2 ==> bab7d34c8b1fc27b7718e8416aa9a39c0430096c (PR)
Upgrade the nodes to version: bab7d34c8b1fc27b7718e8416aa9a39c0430096c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 6936 txn/s, latency: 4782 ms, (p50: 4800 ms, p90: 7700 ms, p99: 8100 ms), latency samples: 242760
5. check swarm health
Compatibility test for aptos-node-v1.7.2 ==> bab7d34c8b1fc27b7718e8416aa9a39c0430096c passed
Test Ok

junkil-park added a commit that referenced this pull request Nov 21, 2023
@junkil-park
[Stake] Allow to set the beneficiary for operator (#10455)
fb6d676 
* Updated `distribute_internal` to use `aptos_account::deposit_coins` instead of `coin::deposit`

* Allows operators to set beneficiaries
@junkil-park junkil-park mentioned this pull request Nov 21, 2023
Merged
junkil-park added a commit that referenced this pull request Nov 22, 2023
@junkil-park
[Stake] Allow to set the beneficiary for operator (#10455)
30c6897 
* Updated `distribute_internal` to use `aptos_account::deposit_coins` instead of `coin::deposit`

* Allows operators to set beneficiaries
sherry-x pushed a commit that referenced this pull request Nov 22, 2023
@junkil-park
[Cherry pick] Allow to set the beneficiary for operator (#11032)
8beaf59 
* [Stake] Allow to set the beneficiary for operator (#10455)

* Updated `distribute_internal` to use `aptos_account::deposit_coins` instead of `coin::deposit`

* Allows operators to set beneficiaries

* Update lib.rs (#10861)

* Update lib.rs (#11029)

This enables OPERATOR_BENEFICIARY_CHANGE in the devent to release (v1.8.4).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@movekevin movekevin movekevin approved these changes

@alexfilip2 alexfilip2 alexfilip2 left review comments

@lightmark lightmark lightmark approved these changes

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@wrwg wrwg Awaiting requested review from wrwg wrwg is a code owner

@michelle-aptos michelle-aptos Awaiting requested review from michelle-aptos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@junkil-park @lightmark @alexfilip2 @movekevin @michelle-aptos