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

Timelocked delegation #162

Merged
merged 13 commits into from
May 9, 2024
Merged

Timelocked delegation #162

merged 13 commits into from
May 9, 2024

Conversation

valeriyr
Copy link
Contributor

@valeriyr valeriyr commented May 3, 2024

Description of change

There are two solution proposals:

  1. The first includes the first two commits and can be used if we wish to change the reward logic or add events, etc.
  2. The second one is a final proposal. We use the existing logic and it is not required to change the original source code, time-locked delegation can be implemented based on it.

Links to any relevant issues

Closes #130
Fixes two tasks of #134

Tests

Move-level integration tests.

@valeriyr valeriyr requested a review from miker83z May 3, 2024 13:08
@valeriyr valeriyr self-assigned this May 3, 2024
@valeriyr valeriyr force-pushed the issue-130-timelocked-delegation branch from c8231bf to 8dabaf6 Compare May 3, 2024 16:20
@valeriyr valeriyr marked this pull request as ready for review May 6, 2024 09:03
let (stake, expire_timestamp_ms) = time_lock::unpack(timelocked_stake);

let staked_sui = sui_system.request_add_stake_non_entry(
stake.into_coin(ctx),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary to get rid of Balance -> Coin conversion.

@miker83z
Copy link
Contributor

miker83z commented May 6, 2024

I suggest to move the modules out from the sui-system, check #166

@valeriyr valeriyr force-pushed the issue-130-timelocked-delegation branch from 7fafe1a to a0e4907 Compare May 7, 2024 09:20
Copy link
Contributor

@miker83z miker83z left a comment

Choose a reason for hiding this comment

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

As discussed, the Timelock and thus also TimelockedStakedSui should be non transferable.

@valeriyr valeriyr requested a review from miker83z May 7, 2024 11:20
Copy link
Contributor

@miker83z miker83z left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@fijter fijter left a comment

Choose a reason for hiding this comment

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

looks good 👍


use sui_system::staking_pool::StakedSui;

const EIncompatibleTimelockedStakedSui: u64 = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const EIncompatibleTimelockedStakedSui: u64 = 1;
const EIncompatibleTimelockedStakedSui: u64 = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

use stardust::timelock::{Self, TimeLock};
use stardust::timelocked_staked_sui::{Self, TimelockedStakedSui};

const ETimeLockShouldNotBeExpired: u64 = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ETimeLockShouldNotBeExpired: u64 = 1;
const ETimeLockShouldNotBeExpired: u64 = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@samuel-rufi samuel-rufi left a comment

Choose a reason for hiding this comment

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

Fantastic PR @valeriyr!

Comment on lines +70 to +92
public(package) fun pack<T: store>(locked: T, expire_timestamp_ms: u64, ctx: &mut TxContext): TimeLock<T> {
// Create a timelock.
TimeLock {
id: object::new(ctx),
locked,
expire_timestamp_ms
}
}

/// An utility function to unpack a `TimeLock`.
public(package) fun unpack<T: store>(lock: TimeLock<T>): (T, u64) {
// Unpack the timelock.
let TimeLock {
id,
locked,
expire_timestamp_ms
} = lock;

// Delete the timelock.
object::delete(id);

(locked, expire_timestamp_ms)
}
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we shouldn't just merge following two function bodies with the respective lock/unlock functions directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samuel-rufi, thank you for the question!

These functions are used internally in the request_add_stake_non_entry and request_withdraw_stake_non_entry ones to create/destroy a TimeLock instance during staking, or in other words, for TimeLock <-> TimeloedStakedSui conversion.
The lock and unlock functions can not be used because they check additional conditions.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for the explaination @valeriyr 👌

@valeriyr valeriyr merged commit c34fe7f into develop May 9, 2024
19 of 35 checks passed
@valeriyr valeriyr deleted the issue-130-timelocked-delegation branch May 9, 2024 07:49
@miker83z miker83z added this to the L1SC/IOTA-Framework milestone May 14, 2024
@miker83z miker83z added the sc-platform Issues related to the Smart Contract Platform group. label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sc-platform Issues related to the Smart Contract Platform group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task (L1SC)]: System State Package/Object review/modification
4 participants