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

Improved role staking #42

Merged
merged 7 commits into from Apr 17, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Expand Up @@ -2,7 +2,7 @@
authors = ['Joystream']
edition = '2018'
name = 'joystream-node-runtime'
version = '5.1.0'
version = '5.2.0'

[features]
default = ['std']
Expand Down
36 changes: 17 additions & 19 deletions src/currency.rs
Expand Up @@ -11,30 +11,28 @@ pub trait GovernanceCurrency: system::Trait + Sized {
pub type BalanceOf<T> =
<<T as GovernanceCurrency>::Currency as Currency<<T as system::Trait>::AccountId>>::Balance;


/// A structure that converts the currency type into a lossy u64
/// And back from u128
pub struct CurrencyToVoteHandler;

impl Convert<u128, u64> for CurrencyToVoteHandler {
fn convert(x: u128) -> u64 {
if x >> 96 == 0 {
x as u64
} else {
u64::max_value()
}
}
fn convert(x: u128) -> u64 {
if x >> 96 == 0 {
x as u64
} else {
u64::max_value()
}
}
}

impl Convert<u128, u128> for CurrencyToVoteHandler {
fn convert(x: u128) -> u128 {
// if it practically fits in u64
if x >> 64 == 0 {
x
}
else {
// 0000_0000_FFFF_FFFF_FFFF_FFFF_0000_0000
u64::max_value() as u128
}
}
}
fn convert(x: u128) -> u128 {
// if it practically fits in u64
if x >> 64 == 0 {
x
} else {
// 0000_0000_FFFF_FFFF_FFFF_FFFF_0000_0000
u64::max_value() as u128
}
}
}
21 changes: 9 additions & 12 deletions src/lib.rs
Expand Up @@ -18,10 +18,7 @@ pub mod currency;
pub mod governance;
use governance::{council, election, proposals};
pub mod storage;
use storage::{
data_directory, data_object_storage_registry, data_object_type_registry,
downloads,
};
use storage::{data_directory, data_object_storage_registry, data_object_type_registry, downloads};
mod membership;
mod memo;
mod traits;
Expand All @@ -41,8 +38,8 @@ use rstd::prelude::*; // needed for Vec
use runtime_primitives::{
create_runtime_str, generic,
traits::{
self as runtime_traits, AuthorityIdFor, BlakeTwo256, Block as BlockT,
DigestFor, NumberFor, StaticLookup, Verify,
self as runtime_traits, AuthorityIdFor, BlakeTwo256, Block as BlockT, DigestFor, NumberFor,
StaticLookup, Verify,
},
transaction_validity::TransactionValidity,
AnySignature, ApplyResult,
Expand Down Expand Up @@ -99,11 +96,11 @@ pub mod opaque {
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
pub struct UncheckedExtrinsic(#[cfg_attr(feature = "std", serde(with = "bytes"))] pub Vec<u8>);
#[cfg(feature = "std")]
impl std::fmt::Debug for UncheckedExtrinsic {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(fmt, "{}", primitives::hexdisplay::HexDisplay::from(&self.0))
}
}
impl std::fmt::Debug for UncheckedExtrinsic {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(fmt, "{}", primitives::hexdisplay::HexDisplay::from(&self.0))
}
}
impl runtime_traits::Extrinsic for UncheckedExtrinsic {
fn is_signed(&self) -> Option<bool> {
None
Expand All @@ -128,7 +125,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("joystream-node"),
impl_name: create_runtime_str!("joystream-node"),
authoring_version: 5,
spec_version: 1,
spec_version: 2,
impl_version: 0,
apis: RUNTIME_API_VERSIONS,
};
Expand Down
62 changes: 39 additions & 23 deletions src/roles/actors.rs
Expand Up @@ -2,7 +2,7 @@ use crate::currency::{BalanceOf, GovernanceCurrency};
use parity_codec_derive::{Decode, Encode};
use rstd::prelude::*;
use runtime_primitives::traits::{As, Bounded, MaybeDebug, Zero};
use srml_support::traits::{Currency, LockIdentifier, LockableCurrency, WithdrawReasons};
use srml_support::traits::{Currency, LockIdentifier, LockableCurrency, WithdrawReason};
use srml_support::{decl_event, decl_module, decl_storage, ensure, StorageMap, StorageValue};
use system::{self, ensure_signed};

Expand Down Expand Up @@ -212,19 +212,29 @@ impl<T: Trait> Module<T> {
role: Role,
member_id: MemberId<T>,
unbonding_period: T::BlockNumber,
stake: BalanceOf<T>,
) {
// simple unstaking ...only applying unbonding period
let value = T::Currency::free_balance(&actor_account);
T::Currency::set_lock(
STAKING_ID,
Self::update_lock(
&actor_account,
value,
stake,
mnaamani marked this conversation as resolved.
Show resolved Hide resolved
<system::Module<T>>::block_number() + unbonding_period,
WithdrawReasons::all(),
);

Self::remove_actor_from_service(actor_account, role, member_id);
}

// Locks account and prevents transfers and reservation. Account still pay basic
// transaction fees and therefore send transactions that don't demand reserving balance
fn update_lock(account: &T::AccountId, stake: BalanceOf<T>, until: T::BlockNumber) {
T::Currency::set_lock(
STAKING_ID,
account,
stake,
until,
WithdrawReason::Transfer | WithdrawReason::Reserve,
mnaamani marked this conversation as resolved.
Show resolved Hide resolved
);
}
}

impl<T: Trait> Roles<T> for Module<T> {
Expand Down Expand Up @@ -277,25 +287,23 @@ decl_module! {
for actor in accounts.into_iter().map(|account| Self::actor_by_account_id(account)) {
if let Some(actor) = actor {
if now > actor.joined_at + params.reward_period {
// send reward to member account - not the actor account
if let Ok(member_account) = T::Members::lookup_account_by_member_id(actor.member_id) {
let _ = T::Currency::deposit_into_existing(&member_account, params.reward);
// reward can top up balance if it is below minimum stake requirement
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the right change, but for two comments:

  1. The reward must still exceed the balance used up during the lifetime of a block for anything to ever be paid out to the member and to guarantee that the actor can continue acting. I'm not sure this is enough, therefore.
  2. The effect of this is that busy actors actually produce less reward for their member than idle actors. I think that's the opposite of what's intended.

Maybe it's enough for getting Athens up and running, because the storage provider incentives are really only properly to be added in future. But it really doesn't seem entirely complete at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed IRL, these edge cases are correct, but would have to be solved by the working group in selecting the best set of role parameters to ensure storage providers are earning more than than their tx fees, as well as introducing a reward model that takes into account how busy the actors are.

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern for Athens is really that storage providers don't run out of balance for transaction fees - I guess it's okay for now to require people to pay attention.

// this guarantees overtime that actor always covers the minimum stake and
// has enough balance to pay for tx fees
let balance = T::Currency::free_balance(&actor.account);
if balance < params.min_stake {
let _ = T::Currency::deposit_into_existing(&actor.account, params.reward);
} else {
// otherwise it should go the the member account
if let Ok(member_account) = T::Members::lookup_account_by_member_id(actor.member_id) {
let _ = T::Currency::deposit_into_existing(&member_account, params.reward);
}
}
}
}
}
}
}

// eject actors not staking the minimum
// iterating over available roles, so if a role has been removed at some point
// and an actor hasn't unstaked .. this will not apply to them.. which doesn't really matter
// because they are no longer incentivised to stay in the role anyway
// TODO: this needs a bit more preparation. The right time to check for each actor is different, as they enter
// role at different times.
// for role in Self::available_roles().iter() {
// }

}

pub fn role_entry_request(origin, role: Role, member_id: MemberId<T>) {
Expand Down Expand Up @@ -349,8 +357,9 @@ decl_module! {

<AccountIdsByRole<T>>::mutate(role, |accounts| accounts.push(actor_account.clone()));
<AccountIdsByMemberId<T>>::mutate(&member_id, |accounts| accounts.push(actor_account.clone()));
let value = T::Currency::free_balance(&actor_account);
T::Currency::set_lock(STAKING_ID, &actor_account, value, T::BlockNumber::max_value(), WithdrawReasons::all());

// Lock minimum stake, but allow spending for transaction fees
Self::update_lock(&actor_account, role_parameters.min_stake, T::BlockNumber::max_value());
<ActorByAccountId<T>>::insert(&actor_account, Actor {
member_id,
account: actor_account.clone(),
Expand All @@ -376,13 +385,20 @@ decl_module! {

let role_parameters = Self::ensure_role_parameters(actor.role)?;

Self::apply_unstake(actor.account.clone(), actor.role, actor.member_id, role_parameters.unbonding_period);
Self::apply_unstake(actor.account.clone(), actor.role, actor.member_id, role_parameters.unbonding_period, role_parameters.min_stake);

Self::deposit_event(RawEvent::Unstaked(actor.account, actor.role));
}

pub fn set_role_parameters(role: Role, params: RoleParameters<T>) {
let new_stake = params.min_stake.clone();
<Parameters<T>>::insert(role, params);
// Update locks for all actors in the role. The lock for each account is already until max_value
mnaamani marked this conversation as resolved.
Show resolved Hide resolved
// It doesn't affect actors which are unbonding, they should have already been removed from AccountIdsByRole
let accounts = Self::account_ids_by_role(role);
for account in accounts.into_iter() {
Self::update_lock(&account, new_stake, T::BlockNumber::max_value());
}
}

pub fn set_available_roles(roles: Vec<Role>) {
Expand All @@ -405,7 +421,7 @@ decl_module! {
let member_id = T::Members::lookup_member_id(&actor_account)?;
let actor = Self::ensure_actor_is_member(&actor_account, member_id)?;
let role_parameters = Self::ensure_role_parameters(actor.role)?;
Self::apply_unstake(actor.account, actor.role, actor.member_id, role_parameters.unbonding_period);
Self::apply_unstake(actor.account, actor.role, actor.member_id, role_parameters.unbonding_period, role_parameters.min_stake);
}
}
}
22 changes: 15 additions & 7 deletions src/storage/data_directory.rs
Expand Up @@ -5,7 +5,7 @@ use parity_codec::Codec;
use parity_codec_derive::{Decode, Encode};
use rstd::prelude::*;
use runtime_primitives::traits::{
As, MaybeDebug, MaybeSerializeDebug, Member, SimpleArithmetic, MaybeDisplay
As, MaybeDebug, MaybeDisplay, MaybeSerializeDebug, Member, SimpleArithmetic,
};
use srml_support::{
decl_event, decl_module, decl_storage, dispatch, ensure, Parameter, StorageMap, StorageValue,
Expand All @@ -17,8 +17,16 @@ pub trait Trait: timestamp::Trait + system::Trait + DOTRTrait + MaybeDebug {

type ContentId: Parameter + Member + MaybeSerializeDebug + MaybeDisplay + Copy + Ord + Default;

type SchemaId: Parameter + Member + SimpleArithmetic + Codec + Default + Copy
+ As<usize> + As<u64> + MaybeSerializeDebug + PartialEq;
type SchemaId: Parameter
+ Member
+ SimpleArithmetic
+ Codec
+ Default
+ Copy
+ As<usize>
+ As<u64>
+ MaybeSerializeDebug
+ PartialEq;

type Members: Members<Self>;
type Roles: Roles<Self>;
Expand Down Expand Up @@ -61,7 +69,6 @@ pub struct DataObject<T: Trait> {
pub size: u64,
pub liaison: T::AccountId,
pub liaison_judgement: LiaisonJudgement,

// TODO signing_key: public key supplied by the uploader,
// they sigh the content with this key

Expand Down Expand Up @@ -290,7 +297,6 @@ impl<T: Trait> ContentIdExists<T> for Module<T> {
}

impl<T: Trait> Module<T> {

fn current_block_and_time() -> BlockAndTime<T> {
BlockAndTime {
block: <system::Module<T>>::block_number(),
Expand Down Expand Up @@ -361,7 +367,8 @@ mod tests {
assert!(res.is_err());

// However, with the liaison as origin it should.
let res = TestDataDirectory::accept_content(Origin::signed(TEST_MOCK_LIAISON), content_id);
let res =
TestDataDirectory::accept_content(Origin::signed(TEST_MOCK_LIAISON), content_id);
assert!(res.is_ok());
});
}
Expand Down Expand Up @@ -389,7 +396,8 @@ mod tests {
assert!(res.is_err());

// However, with the liaison as origin it should.
let res = TestDataDirectory::reject_content(Origin::signed(TEST_MOCK_LIAISON), content_id);
let res =
TestDataDirectory::reject_content(Origin::signed(TEST_MOCK_LIAISON), content_id);
assert!(res.is_ok());
});
}
Expand Down
17 changes: 12 additions & 5 deletions src/storage/data_object_storage_registry.rs
Expand Up @@ -14,8 +14,16 @@ pub trait Trait: timestamp::Trait + system::Trait + DDTrait + MaybeDebug {
type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;

// TODO deprecated
type DataObjectStorageRelationshipId: Parameter + Member + SimpleArithmetic + Codec
+ Default + Copy + As<usize> + As<u64> + MaybeSerializeDebug + PartialEq;
type DataObjectStorageRelationshipId: Parameter
+ Member
+ SimpleArithmetic
+ Codec
+ Default
+ Copy
+ As<usize>
+ As<u64>
+ MaybeSerializeDebug
+ PartialEq;

type Members: Members<Self>;
type Roles: Roles<Self>;
Expand Down Expand Up @@ -43,7 +51,7 @@ pub struct DataObjectStorageRelationship<T: Trait> {

decl_storage! {
trait Store for Module<T: Trait> as DataObjectStorageRegistry {

// TODO deprecated
// Start at this value
pub FirstRelationshipId get(first_relationship_id) config(first_relationship_id): T::DataObjectStorageRelationshipId = T::DataObjectStorageRelationshipId::sa(DEFAULT_FIRST_RELATIONSHIP_ID);
Expand Down Expand Up @@ -84,15 +92,14 @@ decl_event! {
// TODO deprecated
DataObjectStorageRelationshipAdded(DataObjectStorageRelationshipId, ContentId, AccountId),
DataObjectStorageRelationshipReadyUpdated(DataObjectStorageRelationshipId, bool),

// NEW & COOL
StorageProviderAddedContent(AccountId, ContentId),
StorageProviderRemovedContent(AccountId, ContentId),
}
}

impl<T: Trait> ContentHasStorage<T> for Module<T> {

// TODO deprecated
fn has_storage_provider(which: &T::ContentId) -> bool {
let dosr_list = Self::relationships_by_content_id(which);
Expand Down