Second step to remove legacy #159

vzotova · 2023-11-27T22:26:56Z

Now when all legacy token unstaked we don't need any support of it.
All methods related to legacy were removed including new one to force unstake.

github-actions · 2023-11-27T22:28:42Z

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7011406420 check.

github-actions · 2023-11-27T22:49:24Z

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7011594546 check.

github-actions · 2023-11-27T22:50:26Z

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7011601971 check.

michalinacienciala · 2023-11-28T08:21:38Z

Do we still need the @keep-network/keep-core dependency in package.json?

cygnusv · 2023-11-28T09:15:38Z

contracts/staking/IStaking.sol

-    ///         amount to 0 and withdraws all liquid T from the stake to the
-    ///         owner. Reverts if there is at least one non-zero authorization.
+    ///         authorization higher than the remaining liquid T stake or
+    ///         if the unstake amount is higher than the liquid T stake amount.


I think we can change "liquid T stake" (which doesn't make much sense now) into just "T stake" everywhere

cygnusv · 2023-11-28T09:17:06Z

contracts/staking/IStaking.sol

-            uint96 nuInTStake
-        );
+    /// @notice Returns staked amount of T for the specified staking provider.
+    function tStake(address stakingProvider) external view returns (uint96);


What about stakeAmount, or even just stake?

stake is used, I'm fine with stakeAmount

cygnusv · 2023-11-28T09:17:48Z

contracts/staking/TokenStaking.sol

-///         that run on the Threshold Network. Note that legacy NU/KEEP staking
-///         contracts see TokenStaking as an application (e.g., slashing is
-///         requested by TokenStaking and performed by the legacy contracts).
+///         Additionally, it serves as application manager for the apps


Suggested change

/// Additionally, it serves as application manager for the apps

/// It serves as application manager for the apps

cygnusv · 2023-11-28T09:19:58Z

contracts/staking/TokenStaking.sol

@@ -108,7 +101,6 @@ contract TokenStaking is Initializable, IStaking, Checkpoints {
    uint256 public slashingQueueIndex;

    event Staked(
-        StakeType indexed stakeType,


I'm afraid we can't drop this. There's already a bunch of external apps (e.g., Dune queries) that consume events in their current form.

Got it, added back ✔️

cygnusv · 2023-11-28T09:58:18Z

contracts/staking/TokenStaking.sol

        address owner;
-        uint96 keepInTStake;
+        uint96 legacyKeepInTStake;


Can we leave the old names? I ran the OZ upgrade tool on the new contract (see below), and so far the only thing it doesn't like is these 2 name changes. For contract variable names, there's a way to override this error, but for the moment not for struct members ( see https://docs.openzeppelin.com/upgrades-plugins/1.x/faq#how-to-rename). For the moment, instead of changing the name we can leave a comment.

OZ output:

Error: ERROR processing /Users/david/dev/t/solidity-contracts/deploy/60_validate_upgrade_token_staking.ts: Error: New storage layout is incompatible contracts/staking/TokenStaking.sol:120: Upgraded `stakingProviders` to an incompatible type - In mapping(address => struct TokenStaking.StakingProviderInfo) - Bad upgrade to struct TokenStaking.StakingProviderInfo - In struct TokenStaking.StakingProviderInfo - Renamed `nuInTStake` to `legacyNuInTStake` - Renamed `keepInTStake` to `legacyKeepInTStake`

Side-note: I need to add this validation tool to CI, probably I'm doing it as part of #140. I also need to check why the tool doesn't complain about the change in the Staked event signature.

Changing of event can't break storage but renaming theoretically can (if we would use those variables) somewhere.
Renamed back ✔️

vzotova · 2023-11-28T15:39:38Z

Do we still need the @keep-network/keep-core dependency in package.json?

Didn't find any usage so removed, @michalinacienciala could you check please that nothing needed was not removed?

github-actions · 2023-11-28T15:40:13Z

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7021124328 check.

michalinacienciala

Do we still need the @keep-network/keep-core dependency in package.json?

Didn't find any usage so removed, @michalinacienciala could you check please that nothing needed was not removed?

I left two comments. I think we can also do a cleanup of Keep contracts in the ./external folder and remove these lines:

solidity-contracts/hardhat.config.ts

Lines 94 to 95 in 5fb485e

    
           goerli: ["./external/goerli"], 
        
           sepolia: ["./external/sepolia"],

.

.github/workflows/contracts.yml

.github/workflows/npm.yml

github-actions · 2023-11-29T18:06:10Z

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7036583518 check.

michalinacienciala · 2023-12-06T15:11:24Z

hardhat.config.ts

-      goerli: ["./external/goerli"],
-      sepolia: ["./external/sepolia"],


Can we also remove the corresponding folders?

I've noticed only now that I haven't finished with this PR, sorry.
Done! ✔️

manumonti

Bye bye, legacy staking 👋

Please, lmk when the new version of the contract is deployed since it will require some update on subgraphs.

Just a question: why the struct StakingProviderInfo in TokenStaking.sol is still keeping nuInTStake and keepInTStake?

vzotova · 2024-03-13T13:18:43Z

Bye bye, legacy staking 👋

Please, lmk when the new version of the contract is deployed since it will require some update on subgraphs.

Just a question: why the struct StakingProviderInfo in TokenStaking.sol is still keeping nuInTStake and keepInTStake?

otherwise data layout will be corrupted and other values would not be get correctly. knowing that those slots are 0 we can theoretically reuse them for something new but I'd avoid it too

theref

i love it 🪓

Co-authored-by: David Núñez <[email protected]> Co-authored-by: Michalina <[email protected]>

…m governance contracts Co-authored-by: Michalina <[email protected]>

Co-authored-by: Michalina <[email protected]>

cygnusv · 2024-09-09T17:33:51Z

contracts/staking/TokenStaking.sol

-    /// @notice Returns staked amount of T, Keep and Nu for the specified
-    ///         staking provider.
-    /// @dev    All values are in T denomination
-    function stakes(address stakingProvider)


I just realized we will be breaking stuff that consumes Threshold staking information by removing this method. For example, here's the dashboard unpacking the result from stakes():
https://github.com/threshold-network/token-dashboard/blob/8264be8e00286f9d51674fd07ca67030a40ed168/src/threshold-ts/staking/index.ts#L220

What do you think about keeping this method, but marking it as deprecated? Still, I'd keep the stakeAmount() method. In the meantime, we can identify stuff that can be updated or removed in the dashboard after legacy support was dropped, which I think it's probably a lot.

cygnusv · 2024-09-09T17:45:23Z

docs/rfc-1-staking-contract.adoc

@@ -369,27 +342,21 @@ each affected application.

 Returns the authorized stake amount of the staking provider for the application.

-==== `stakes(address stakingProvider) external view returns (uint96 tStake, uint96 keepInTStake, uint96 nuInTStake)`


See my comment above about leaving this method but marked as deprecated

cygnusv · 2024-09-09T17:45:40Z

docs/rfc-1-staking-contract.adoc

-the specified stake type is the minimum amount of stake of the given type needed
-to satisfy the maximum application authorization given the staked amounts of the
-T stake type for that staking provider.
+Returns the maximum application authorization


Suggested change

Returns the maximum application authorization

Returns the maximum application authorization.

Co-authored-by: David Núñez <[email protected]>

vzotova requested review from cygnusv, pdyraga, lukasz-zimnoch, derekpierre and a team November 27, 2023 22:26

vzotova self-assigned this Nov 27, 2023

vzotova force-pushed the remove-legacy-step-2 branch 2 times, most recently from edfbc18 to b181f2a Compare November 27, 2023 22:48

cygnusv requested changes Nov 28, 2023

View reviewed changes

michalinacienciala requested changes Nov 29, 2023

View reviewed changes

.github/workflows/contracts.yml Outdated Show resolved Hide resolved

.github/workflows/npm.yml Outdated Show resolved Hide resolved

vzotova mentioned this pull request Nov 29, 2023

TokenStaking: method to increase authorization to max #161

Open

michalinacienciala reviewed Dec 6, 2023

View reviewed changes

manumonti previously approved these changes Mar 13, 2024

View reviewed changes

vzotova dismissed manumonti’s stale review via fe1353a March 13, 2024 13:20

vzotova force-pushed the remove-legacy-step-2 branch from 213f0bb to fe1353a Compare March 13, 2024 13:20

theref previously approved these changes Mar 13, 2024

View reviewed changes

vzotova mentioned this pull request Mar 23, 2024

Adds tracking of overall authorized amount for each application #166

Open

manumonti previously approved these changes Mar 26, 2024

View reviewed changes

vzotova and others added 3 commits September 9, 2024 12:42

TokenStaking: removes all functions related to legacy stakes

d778521

Apply suggestions from code review

e4ec5ae

Co-authored-by: David Núñez <[email protected]> Co-authored-by: Michalina <[email protected]>

Updates github workflow and hardhat config, removes "legacy" info fro…

b20c442

…m governance contracts Co-authored-by: Michalina <[email protected]>

Removes unused deploymentsL sepolia and goerli

63bcfc3

Co-authored-by: Michalina <[email protected]>

vzotova force-pushed the remove-legacy-step-2 branch from fe1353a to 63bcfc3 Compare September 9, 2024 16:42

cygnusv reviewed Sep 9, 2024

View reviewed changes

Apply suggestions from code review

9d5daff

Co-authored-by: David Núñez <[email protected]>

vzotova dismissed stale reviews from manumonti and theref via 9d5daff September 9, 2024 19:30

theref approved these changes Sep 10, 2024

View reviewed changes

cygnusv approved these changes Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Second step to remove legacy #159

Second step to remove legacy #159

vzotova commented Nov 27, 2023

github-actions bot commented Nov 27, 2023

github-actions bot commented Nov 27, 2023

github-actions bot commented Nov 27, 2023

michalinacienciala commented Nov 28, 2023

cygnusv Nov 28, 2023

vzotova Nov 28, 2023

cygnusv Nov 28, 2023

vzotova Nov 28, 2023

vzotova Nov 28, 2023

cygnusv Nov 28, 2023

vzotova Nov 28, 2023

cygnusv Nov 28, 2023

vzotova Nov 28, 2023

cygnusv Nov 28, 2023 •

edited

Loading

vzotova Nov 28, 2023

vzotova commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023

michalinacienciala left a comment

github-actions bot commented Nov 29, 2023

michalinacienciala Dec 6, 2023

vzotova Mar 12, 2024

manumonti left a comment

vzotova commented Mar 13, 2024

theref left a comment

cygnusv Sep 9, 2024

vzotova Sep 9, 2024

cygnusv Sep 9, 2024

vzotova Sep 9, 2024

cygnusv Sep 9, 2024

vzotova Sep 9, 2024

	/// Additionally, it serves as application manager for the apps
	/// It serves as application manager for the apps

	goerli: ["./external/goerli"],
	sepolia: ["./external/sepolia"],

		@@ -369,27 +342,21 @@ each affected application.

		Returns the authorized stake amount of the staking provider for the application.

		==== `stakes(address stakingProvider) external view returns (uint96 tStake, uint96 keepInTStake, uint96 nuInTStake)`

	Returns the maximum application authorization
	Returns the maximum application authorization.

Second step to remove legacy #159

Are you sure you want to change the base?

Second step to remove legacy #159

Conversation

vzotova commented Nov 27, 2023

github-actions bot commented Nov 27, 2023

github-actions bot commented Nov 27, 2023

github-actions bot commented Nov 27, 2023

michalinacienciala commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cygnusv Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzotova commented Nov 28, 2023 • edited Loading

github-actions bot commented Nov 28, 2023

michalinacienciala left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manumonti left a comment

Choose a reason for hiding this comment

vzotova commented Mar 13, 2024

theref left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cygnusv Nov 28, 2023 •

edited

Loading

vzotova commented Nov 28, 2023 •

edited

Loading