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

[sui-framework] Remove "delegate" and replace with stake #9059

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

tzakian
Copy link
Contributor

@tzakian tzakian commented Mar 9, 2023

This does a large rename of "delegation/delegate" (etc) to the equivalent "stake" name, e.g., request_add_delegation becomes request_add_stake.

Move

Entry functions renamed (all within sui_system.move):

request_add_delegation ~> request_add_stake
request_add_delegation_mul_coin ~> request_add_stake_mul_coin
request_add_delegation_with_locked_coin ~> request_add_stake_with_locked_coin
request_add_delegation_with_mul_locked_coin ~> request_add_stake_with_mul_locked_coin
request_withdraw_delegation ~> request_withdraw_stake

Move Object fields

StakingPool::pending_delegation ~> pending_stake
StakedSui::delegation_activation_epoch ~> stake_activation_epoch 

Sui types

This renames a couple sui types since they no longer made sense to be called what they were:

DelegatedStake ~> Stake
DelegationStatus ~> StakeStatus

This then performs the core set of renamings to make the system happy with this. So there are still a number of functions in Rust and Typescript that will contain their old delegation name.

Note that on the typescript side we keep DelegatedStake since (1) there was already a type called Stake (which meant something else) and in the context that I could see it being used it appeared as though DelegatedStake really did mean "this is my stake that I have delegated to this validator."

Test Plan

Make sure existing tests still pass.


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

@vercel
Copy link

vercel bot commented Mar 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
explorer ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 6:55PM (UTC)
explorer-storybook ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 6:55PM (UTC)
wallet-adapter ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 6:55PM (UTC)

@github-actions github-actions bot added the Type: Documentation Improvements or additions to documentation label Mar 9, 2023
@patrickkuo
Copy link
Contributor

the governance api PR #8848 has some of the same renaming changes on the RPC layer, SDK and the FE apps, this will create quite a bit of conflicts, can we put this on hold 🙏?

/// Add delegated stake to a validator's staking pool using multiple coins.
public entry fun request_add_delegation_mul_coin(
/// Add stake to a validator's staking pool using multiple coins.
public entry fun request_add_stake_mul_coin(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really just remove this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep things clean I might leave this in for now so this is a strict renaming pass, but will do a fast follow to remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I believe this function is used in our sui-transaction-builder as well as sui-rosetta:

ADD_DELEGATION_MUL_COIN_FUN_NAME

cc @patrickkuo

@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from 6147162 to f48e225 Compare March 10, 2023 03:40
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 03:40 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 03:41 Inactive
@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from f48e225 to 90fa9f0 Compare March 10, 2023 03:50
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 03:51 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 03:52 Inactive
@tzakian tzakian requested a review from sblackshear March 10, 2023 03:54
@tzakian
Copy link
Contributor Author

tzakian commented Mar 10, 2023

The plan is to wait for #9058 to land, and once it does this PR will be rebased on that and then landed.

In the meantime please review and let me know if there is anywhere I have missed or that you think an error was made.

@tzakian tzakian marked this pull request as ready for review March 10, 2023 03:56
@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from 90fa9f0 to e3c6945 Compare March 10, 2023 17:07
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 17:08 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 17:08 Inactive
@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from e3c6945 to 1176fa1 Compare March 10, 2023 17:54
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 17:55 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 17:55 Inactive
@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from 1176fa1 to 0153f2d Compare March 10, 2023 18:17
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 18:18 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 18:18 Inactive
@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from 0153f2d to c1aa325 Compare March 10, 2023 18:24
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 18:25 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 18:25 Inactive
@@ -578,7 +578,7 @@ module sui::sui_system {
/// 1. Add storage charge to the storage fund.
/// 2. Burn the storage rebates from the storage fund. These are already refunded to transaction sender's
/// gas coins.
/// 3. Distribute computation charge to validator stake and delegation stake.
/// 3. Distribute computation charge to validator stake and stake stake.
Copy link
Contributor

Choose a reason for hiding this comment

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

stake stake 🤣 but let's land the PR first and fix this later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this at the same time as I was reverting the safe_tests file.

Copy link
Contributor

@emmazzz emmazzz left a comment

Choose a reason for hiding this comment

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

We need to revert the changes to one file but other than that LGTM, approving to unblock. Thank you so much! @tzakian

@tzakian tzakian force-pushed the tzakian/remove-delegate-naming branch from c1aa325 to 4a8c6e5 Compare March 10, 2023 18:54
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 10, 2023 18:55 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 10, 2023 18:55 Inactive
@tzakian tzakian enabled auto-merge (squash) March 10, 2023 18:55
@tzakian tzakian merged commit 0a7b42a into main Mar 10, 2023
@tzakian tzakian deleted the tzakian/remove-delegate-naming branch March 10, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants