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

feat: MMD-1309 10-to-1 #225

Draft
wants to merge 73 commits into
base: luke/MMD-1309/update-substrate-3
Choose a base branch
from

Conversation

ltfschoen
Copy link
Collaborator

@ltfschoen ltfschoen commented Sep 8, 2021

Note: To debug this branch run the following:

  • Clone branch and build
git fetch origin luke/rewards-allowance-new:luke/rewards-allowance-new
git checkout luke/rewards-allowance-new
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
rustup toolchain install nightly-2021-08-31
rustup default nightly-2021-08-31
rustup override set nightly-2021-08-31
rustup target add wasm32-unknown-unknown --toolchain nightly-2021-08-31
cargo build --release

IMPORTANT: If you get error duplicate lang item in crate, it's likely because I've added println! statements in a pallet for debugging during testing when running cargo test..., so you'll have to temporarily comment those println! statements out to build

  • Run tests (check that you've first uncommented the println! statements to view debug logs during testing)
SKIP_WASM_BUILD=1 RUST_LOG=runtime=debug cargo +nightly-2021-08-31 test --package mining-rewards-allowance -- --nocapture

and use println in implementation to add variables to log i.e. println!("block: {:#?}, miner: {:#?}, date_start: {:#?}", _n, miner_count, start_of_requested_date_millis);

  • Run a dev chain validator node and view logs to see if what we've setup is working (for example the two default registered DHX Miners that we've setup in the genesis)
RUST_LOG=info RUST_BACKTRACE=1 ./target/release/datahighway --validator \
  --unsafe-ws-external \
  --unsafe-rpc-external \
  --rpc-cors=all \
  --base-path /tmp/polkadot-chains/alice \
  --chain dev \
  --node-key 88dc3417d5058ec4b4503e0c12ea1a0a89be200fe98922423d4334014fa6b0ee \
  --alice \
  --name "dev Validator Alice" \
  --port 30333 \
  --ws-port 9944 \
  --rpc-port 9933 \
  --telemetry-url "wss://telemetry.polkadot.io/submit/ 0" \
  --execution=native \
  -lruntime=debug \
  --rpc-methods=Unsafe

Configure runtime for faster debugging to check functionality still works

Since we want simulate bonding for debugging, we want to the sessions and eras to go by faster, so we can change these runtime values:

See #218

pub const EPOCH_DURATION_IN_BLOCKS: BlockNumber = 1 * MINUTES;

pub const SessionsPerEra: sp_staking::SessionIndex = 1;
pub const BondingDuration: pallet_staking::EraIndex = 1;
pub const SlashDeferDuration: pallet_staking::EraIndex = 1;

For faster democracy proposals and referendums:

pub const LaunchPeriod: BlockNumber = 2 * MINUTES;
pub const VotingPeriod: BlockNumber = 2 * MINUTES;
pub const FastTrackVotingPeriod: BlockNumber = 2 * MINUTES;
pub const EnactmentPeriod: BlockNumber = 2 * MINUTES;
pub const CooloffPeriod: BlockNumber = 2 * MINUTES;

After making changes, build cargo build --release

Outstanding issues

  • - Error when building with cargo build --release
error[E0277]: the trait bound `&BondedDHXForAccountData<<T as frame_system::Config>::AccountId, <<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance, <T as frame_system::Config>::AccountId>: EncodeLike<<<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance>` is not satisfied
   --> pallets/mining/rewards-allowance/src/lib.rs:282:84
    |
282 |             <RewardsAllowanceDHXForDate<T>>::insert(requested_date_millis.clone(), &bonded_data);
    |                                                                                    ^^^^^^^^^^^^ the trait `EncodeLike<<<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance>` is not implemented for `&BondedDHXForAccountData<<T as frame_system::Config>::AccountId, <<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance, <T as frame_system::Config>::AccountId>`
    |
note: required by `frame_support::pallet_prelude::StorageMap::<Prefix, Hasher, Key, Value, QueryKind, OnEmpty, MaxValues>::insert`
   --> /Users/ls2/.cargo/git/checkouts/substrate-7e08433d4c370a21/852bab0/frame/support/src/storage/types/map.rs:142:2
    |
142 |     pub fn insert<KeyArg: EncodeLike<Key>, ValArg: EncodeLike<Value>>(key: KeyArg, val: ValArg) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
    |
244 |     impl<T: Config> Pallet<T> where &BondedDHXForAccountData<<T as frame_system::Config>::AccountId, <<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance, <T as frame_system::Config>::AccountId>: EncodeLike<<<T as pallet::Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance> {
    |                               ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Update: Thanks to Guillaume I discovered this was because I was incorrectly storing the data into RewardsAllowanceDHXForDate when i meant to be storing it in BondedDHXForAccountForDate, and this was accepting a Vec, which i didn't need

  • - Error when trying to run the unit tests with cargo +nightly-2021-08-31 test -p mining-rewards-allowance. To generate the error it's first necessary to comment out the extrinsic function called set_bonded_dhx_of_account_for_date
thread 'tests::it_sets_rewards_allowance_with_timestamp' panicked at 'Every active pallet has a name in the runtime; qed', pallets/mining/rewards-allowance/src/lib.rs:91:15
stack backtrace:
   0: rust_begin_unwind
             at /rustc/5d6804469d80aaf26f98090ae016af45e267f58f/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at /rustc/5d6804469d80aaf26f98090ae016af45e267f58f/library/core/src/panicking.rs:101:14
   2: core::option::expect_failed
             at /rustc/5d6804469d80aaf26f98090ae016af45e267f58f/library/core/src/option.rs:1618:5
   3: core::option::Option<T>::expect
             at /rustc/5d6804469d80aaf26f98090ae016af45e267f58f/library/core/src/option.rs:698:21
   4: <pallet_mining_rewards_allowance::pallet::_GeneratedPrefixForStorageRewardsAllowanceDHXCurrent<T> as frame_support::traits::storage::StorageInstance>::pallet_prefix
             at ./src/lib.rs:91:15
   5: <frame_support::storage::types::value::StorageValue<Prefix,Value,QueryKind,OnEmpty> as frame_support::storage::generator::value::StorageValue<Value>>::module_prefix

Update: Thanks to Guillaume this error was fixed in this commit 1b410fc

  • - trying to mock the Scheduler pallet (since it is used by the Democracy pallet). i've tried to mock it the same way as in the Substrate codebase, but i get the following error. I've reached out fo Bryan Chen for help
error[E0277]: the trait bound `mock::Origin: From<RawOrigin<u64>>` is not satisfied
   --> pallets/mining/rewards-allowance/src/mock.rs:126:5
    |
126 |     type ScheduleOrigin = EnsureOneOf<u64, EnsureRoot<u64>, EnsureSignedBy<OneToFive, u64>>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<RawOrigin<u64>>` is not implemented for `mock::Origin`
    |
    = help: the following implementations were found:
              <mock::Origin as From<OriginCaller>>
              <mock::Origin as From<RawOrigin<u128>>>
              <mock::Origin as From<std::option::Option<u128>>>
    = note: required because of the requirements on the impl of `EnsureOrigin<mock::Origin>` for `EnsureOneOf<u64, EnsureRoot<u64>, EnsureSignedBy<OneToFive, u64>>`
note: required by a bound in `pallet_scheduler::Config::ScheduleOrigin`
   --> /Users/ls2/.cargo/git/checkouts/substrate-7e08433d4c370a21/852bab0/frame/scheduler/src/lib.rs:160:24
    |
160 |         type ScheduleOrigin: EnsureOrigin<<Self as system::Config>::Origin>;
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `pallet_scheduler::Config::ScheduleOrigin`

error[E0277]: the trait bound `Result<RawOrigin<u64>, mock::Origin>: From<mock::Origin>` is not satisfied
   --> pallets/mining/rewards-allowance/src/mock.rs:126:5
    |
126 |     type ScheduleOrigin = EnsureOneOf<u64, EnsureRoot<u64>, EnsureSignedBy<OneToFive, u64>>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<mock::Origin>` is not implemented for `Result<RawOrigin<u64>, mock::Origin>`
    |
    = help: the following implementations were found:
              <Result<(), sp_runtime::DispatchError> as From<sp_runtime::DispatchError>>
              <Result<RawOrigin<u128>, mock::Origin> as From<mock::Origin>>
              <Result<ValidTransaction, TransactionValidityError> as From<InvalidTransaction>>
              <Result<ValidTransaction, TransactionValidityError> as From<UnknownTransaction>>
              <Result<ValidTransaction, TransactionValidityError> as From<ValidTransactionBuilder>>
    = note: required because of the requirements on the impl of `Into<Result<RawOrigin<u64>, mock::Origin>>` for `mock::Origin`
    = note: required because of the requirements on the impl of `EnsureOrigin<mock::Origin>` for `EnsureOneOf<u64, EnsureRoot<u64>, EnsureSignedBy<OneToFive, u64>>`
note: required by a bound in `pallet_scheduler::Config::ScheduleOrigin`
   --> /Users/ls2/.cargo/git/checkouts/substrate-7e08433d4c370a21/852bab0/frame/scheduler/src/lib.rs:160:24
    |
160 |         type ScheduleOrigin: EnsureOrigin<<Self as system::Config>::Origin>;
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `pallet_scheduler::Config::ScheduleOrigin`

Update: the error i was getting was because i needed to change ScheduleOrigin = EnsureRoot<u64>; to type ScheduleOrigin = EnsureRoot<u128>; in pallet_scheduler. i guess that's because we're using an AccountId of u128 in frame_system since according to the Substrate comments u64 is not enough to hold bytes used to generate bounty account. See how Bryan Chen did it here https://github.com/open-web3-stack/open-runtime-module-library/blob/master/authority/src/mock.rs#L62

  • - Mock the following pallets in the mining-rewards-allowances pallet. Reason: we want to write tests that simulates users participating in democracy (using democracy pallet) by locking a deposit of tokens for proposal voting, or participating in elections (using elections-phragmen pallet) by bonding a deposit of tokens to back some candidates (i.e. election member deposit, candidate deposit, or a stake placed on a vote), then i want to query that data since our goal is to reward users that are "participating" through these means.

    • - System, Balances, Transaction Payment
    • - Timestamp, Aura
    • - Treasury, Tips, Bounty
    • - Democracy, Collective, Membership, Scheduler
    • - Elections Phragmen
      Draft: 71b716a
  • Fixed the test suite which was using u64 for the Balance type, which differed from the implementation that was using u128 for the Balance type, so conversion between u128 to Balance type when running tests was failing

  • Wrote a test for the full flow of distributing rewards to registered DHX miners after satisfying their cooling off period and checking storage was updated with reward information. This is to protect the code so it doesn't break when I start introducing future changes like integrating mPower rewards.

  • check that daily reward quotas are reset each day so eligible receive rewards two days or more in a row

  • check that if they unbond so insufficient bonded then they stop receiving the rewards the next day onward and cooldown period starts

  • check that after 3 months (we’ll make it say 3 days in the tests) the multiplier doubles

  • check that after the multiplier doubles, they are no longer eligible to receive the rewards if they have the same amount bonded (since they’d need twice the amount bonded), even if they have sufficient mpower, and that multiplier period days total and remaining are reset. note: we've just added a test that sets their bonded amount to 0 and checked that their accumulated and aggregated rewards become 0 and their cool down period starts.

  • check that if insufficient mPower then they stop receiving rewards the next day and cooldown period starts

  • check that pause/reset works

  • - Write tests for the flow of simulating user accounts participating by bonding DHX tokens. Update: it appears to be sufficient to check Balance::locks, this should do for the moment

i.e.

democracy
	// those who have a locked deposit
	depositOf(PropIndex)

	// public proposals unsorted 
	publicProps() -> Vec<(PropIndex, Hash, AccountId)

	// accounts where locks are in action that may be removed in future
	locks(AccountId) -> Option<BlockNumber>

	// all votes of a particular voter
	votingOf(AccountId) -> Voting

elections-phragmen
	// ** election candidates must have bonded a value of frame_support > `CandidacyBond`
	candidate_ids()...
		Elections::candidates()...

	candidate_deposit(who)
		Elections::candidates()...

	// amount of stake placed on vote
	Voter > stake

	// amount of deposit reserved for vote
	// voters must have bonded a frame_support > `VotingBondBase` & `VoterBondFactor`
	Voter > deposit

	// election candidates must have bonded a value of frame_support > `CandidacyBond`
	Elections.is_candidate()
	... > is_member()
	Elections.members()
	... > is_runner_up()
	... > members_ids()
	// election members must have a stake
	... > members_and_stake()
	... > contains(AccountId)
	... > sorted_members()
	... > all_voters()
	// balance locked in an election
	... > has_lock(AccountId)
	// voters locked stake
	... > locked_stake_of(AccountId)
	... > votes_of(AccountId)
  • - every 3 months initially we'll change from 10:1 to 20:1 and so on, it may be changed by governance proposal. this is part of migrating the dhx center temporary solution onto blockchain so it's decentralised. the bulk of this functionality and tests to provide robustness was finished in commit 2211881

  • check that rewards are distributed after cooling off period.
    note: this will be done after we've implemented off-chain workers for mpower, and after moving the distribution of rewards into a "claim" extrinsic that the user needs to call (instead of being in the on_initialize or on_finalize function)

  • -
    issue of if set mpower in storage near end of date, then set bonded dhx shortly after, then they are stored under
    keys on different days and doesn't match for rewards!!
    issue of if we can't get their off-chain mPower today due to network issues retrieving it, then we'll need to still store a "pending" status until we receive the mPower for that date, and when we do we'll calculate and update the accumulated/aggregated for that day and let them be eligible to claim rewards if there were still dhx remaining for that date.
    so we need to also loop through the flagged historic dates for all the miners were we couldn't get their mpower too somehow.
    in the meantime we'll assume we always get the mpower in time

why do i have to do change_mpower_for_each_miner on the day beforehand i do accum/aggreg for it to work?

we need to set the mPower for the next start date so it's available from off-chain in time,
we really should store the next start date as the day that we actually process the
previous day's eligibility for rewards that were accum/aggregated since we need to allow time to get the mPower first

  • - Instead of storing data on chain, just emit events, and use SubQuery tool as an indexer, which is like TheGraph to do things like taking snapshots every date (instead of having to use Polkadot.js), so we only need to write the indexer itself, then query data + storage at a block, so we could run the indexer all the time, and get data we want at end of the month to distribute rewards based on events that were emitted. It's like substrate-archive, you go through blocks to get the data, then query the database (like you would with RDMS) so it's better than an API and you can do aggregation of the data. There may even be SaaS services that we could use to do the queries.
    Note: Each pallet can have different locks (i.e. Democracy::locks) that apply in parallel to prevent the user spending above the max. lock, where those locks share the same balance of Balances::locks
    Note: As a rule of thumb, don't store anything non-consensus on-chain. (i.e. balances related to consensus, need read both balances since its the input, but system > remark isn't related to consensus, so we don't want to trash the blockchain by storing that information on the chain.

  • Note: to test that the functionality works using log library whilst running a node where it reduces remaining days and issues rewards all on the same day in subsequent blocks (instead of only doing it once per day, which takes too long to check that it works), then just comment out the lines like cooling_off_period_days_remaining.0 != start_of_requested_date_millis.clone() && and if rm_current_period_days_remaining.1 != start_of_requested_date_millis.clone() { (including this line's end brace), and change from let mut locks_first_amount_as_u128 = 0u128.clone(); to let mut locks_first_amount_as_u128 = default_bonded_amount_u128.clone();

  • - add offchain_worker function to runtime, since we'll need to use this instead of on_initialize and on_finalize for some stuff

  • - in the Substrate fork that we're using as a dependency, we're exposing some functions publicly only for testing that we don't want to be exposed in production (i.e. the ability to just call a function to create a referendum directly), how do we restrict it so they may only be used for testing? (i.e. use our Substrate fork commit dependencies only for testing, and use the normal Substrate commit dependencies for production)???

…e are giving them each a proportion of the daily rewards available instead of first processed first served
…gging the Balance type issue, where it should have been u128 instead of u64 in the mocks
…and rewards_allowance_dhx_daily so not reducing remaining. fix by funding treasury in tests so rewards paid
…and cool down starts. initialise default bonded amounts to 0 in implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant