vortex token distribution #726

leo-plugdefi · 2023-10-27T20:31:19Z

add multisig in runtime
configure runtime
rebase on latest main when ready (including upgrade dependencies)
code improvement
proper benchmark & weight will be triggered in workflow
e2e test script

This pallet is for automatic vortex calculation, distribution.
Due to the time restraint staking related calculation is done offchain and reward data has to be fed by privileged account.
Asset price is also set by privileged account.

zees-dev · 2023-10-29T20:59:34Z

pallet/vortex-distribution/src/lib.rs

+	pub struct Pallet<T>(_);
+
+	#[pallet::storage]
+	#[pallet::getter(fn vtx_dist_statuses)]


nit: we try avoid using storage getters due to the fact there is more than 1 way to access storage (and hence makes it more difficult to search for all storage references).

same comment applies for the storage getters below

I was told these have been changed.

zees-dev · 2023-10-29T21:01:28Z

pallet/vortex-distribution/src/lib.rs

+	impl<T: Config> Pallet<T> {
+		#[pallet::weight(<T as pallet::Config>::WeightInfo::list_vtx_dist())]
+		#[transactional]
+		pub fn list_vtx_dist(


nit: should probably rename this extrinsic; it's not a list operation - but instead performs state changes.
maybe;

Suggested change

pub fn list_vtx_dist(

pub fn set_vortex_amount(

ken-futureverse · 2023-11-01T01:01:57Z

Add screenshot to explain the current functionality of the Vortex pallet

JasonTulp

Hey Leo, have had a chance to look through and I can't see any major flaws in the logic so far. A few things are missing from this PR which may already be addressed but thought I'd mention them as they weren't mentioned in your list.

Doc comments are missing throughout. They should be on every extrinsic, event, error config trait and helper function
Missing unit tests, every extrinsic should have a unit test for the best case and all failure cases
Input parameters for some extrinsics are unbounded (i.e. using Vec instead of BoundedVec)
Events should be declared with named parameters. i.e.
VtxDistEnabled{ identifier: T::VtxDistIdentifier }
Benchmarks should include the verify part where applicable

Once the full changes are in place I will give a more complete review and pick out any smaller issues that may still be present.

pallet/vortex-distribution/src/lib.rs

surangap · 2023-11-02T02:01:56Z

pallet/vortex-distribution/src/lib.rs

+
+	#[pallet::pallet]
+	#[pallet::generate_store(pub(super) trait Store)]
+	#[pallet::without_storage_info]


why do we need this?

to quote We need this as there are a couple of storages use which doesn't implement MaxEncodedLen. Most of them are updated to use BoundedVec, but still get one leftover - VtxDistPayoutPivot, needs to find a proper size limitation for the length. With this macro, it'd stop propogate metadata for the pallet storage which would reduce WASM size a bit.

yep, the question is more about whether can we remove it. Even though substrate state db raw key length can have varying lengths, we should be able to define a bound for this case considering that it's going to be raw keys from VtxDistOrderbook that get stored in VtxDistPayoutPivot?
If we plan to migrate to a bounded vec once it's deployed, there might be migrations involved. we should do it now if we can

e2e/common/index.ts

evm-precompiles/futurepass/README.md

runtime/src/weights/mod.rs

runtime/src/constants.rs

runtime/src/lib.rs

leo-plugdefi · 2023-11-08T03:50:31Z

Hey Leo, have had a chance to look through and I can't see any major flaws in the logic so far. A few things are missing from this PR which may already be addressed but thought I'd mention them as they weren't mentioned in your list.
* Doc comments are missing throughout. They should be on every extrinsic, event, error config trait and helper function

* Missing unit tests, every extrinsic should have a unit test for the best case and all failure cases

* Input parameters for some extrinsics are unbounded (i.e. using Vec instead of BoundedVec)

* Events should be declared with named parameters. i.e.
  `VtxDistEnabled{ identifier: T::VtxDistIdentifier }`

* Benchmarks should include the verify part where applicable
Once the full changes are in place I will give a more complete review and pick out any smaller issues that may still be present.

I was told these has been addressed

pallet/vortex-distribution/src/lib.rs

surangap · 2023-11-09T00:44:10Z

pallet/vortex-distribution/src/lib.rs

+						// let total_vortex: <T as Config>::CurrencyBalance =
+						// T::MultiCurrency::total_issuance(T::VtxAssetId::get()); log::warn!("
+						// payed, vortex_balance is {:?}, total_vortex is {:?}", share,
+						// total_vortex);


nit - commented code

zees-dev · 2023-11-09T00:45:03Z

e2e/test/Vortex.test.ts

+  });
+});
+
+const finalizeTx = async (signer: KeyringPair, extrinsic: SubmittableExtrinsic<"promise">) => {


why not import the function from utils?

zees-dev · 2023-11-09T00:45:50Z

e2e/test/Vortex.test.ts

I haven't run this test; but is this test repeatable?
Ideally all tests should be repeatable if possible - otherwise it's really hard to debug broken tests since that would require node restarts :/

zees-dev · 2023-11-09T00:46:40Z

e2e/hardhat.config.ts

@@ -45,7 +45,7 @@ const config: HardhatUserConfig = {
    apiKey: process.env.ETHERSCAN_API_KEY,
  },
  mocha: {
-    timeout: 120_000, // global tests timeout
+    timeout: 12_000_000, // global tests timeout


what is the unit for this? 12,000s? does this not seem very long?

pallet/vortex-distribution/src/lib.rs

surangap · 2023-11-09T01:27:15Z

pallet/vortex-distribution/src/tests.rs

+			assert_eq!(AssetsExt::balance(usdc, &bob), 500_000);
+			assert_eq!(Balances::free_balance(&bob), 500_000);


Add the calculation or a comment.
also a test/check for root reward?

surangap · 2023-11-09T01:32:05Z

runtime/src/lib.rs

+parameter_types! {
+  pub const VortexPalletId: PalletId = PalletId(*b"root/vtx");
+  pub const UnsignedInterval: BlockNumber =  MINUTES / 2;
+  pub const PayoutBatchSize: u32 =  799;


any specific reason for 799 ?

runtime/src/weights/pallet_multisig.rs

JasonTulp

Hey Leo, I've had a thorough look through with the focus on determining which tests need to be added as there are still not nearly enough unit tests to meet the standard we adhere by in our PRs. I am in the process of testing these changes locally so will add more comments if anything else comes up.

One other point outside of lack of unit tests is would we not need a migration for the Vortex AssetId? I see it is created in the genesis config, however when updating the runtime this will not be set. I think we'd need a small migration with all the metadata for the Vortex token and any initial issuance

Below are the tests that are missing from this PR:

create_vst_dist extrinsic:

Failure case for if called from not the VtxDistAdmin
Isolated success case for ensuring storage is updated and event thrown
Failure case for if VtxDistStatuses does not contain the VtxDistIdentifier

disable_vtx_dist extrinsic:

Failure case for if called from not the VtxDistAdmin
Isolated success case for ensuring storage is updated and event thrown

start_vtx_dist extrinsic:

Failure case for if called from not the VtxDistAdmin
Isolated success case for ensuring storage is updated and event thrown

pay_unsigned:

Failure case for if called by a signed address
Ensure payment does not happen if the VtxDistStatus is not == VtxDistStatus::Paying
Ensure payment does not happen if there is no vault account
Ensure the correct accounts are paid out based on the VtxDistPayoutPivot. This should
ensure the orderbook is updated accordingly
- This should also ensure the VtxDistPaidOut event is thrown for every account we
  expect to be paid out and the VtxDistDone event is thrown when complete
- Should also do a test that spans multiple payout blocks. i.e.e the next_unsigned_at block is then checked and the remainder paid out
- There should be a separate test for both with and without a start key

set_vtx_dist_eras extrinsic:

Failure case for if called from not the VtxDistAdmin
Isolated success case for ensuring storage is properly updated (This extrinsic also needs an event to be thrown)
Failure case when the start_era is greater then the end_era

set_asset_prices extrinsic:

Failure case for if called from not the VtxDistAdmin
Isolated success case for ensuring storage is properly updated for all assets (This extrinsic also needs an event to be thrown)

register_rewards extrinsic:

Failure case for if called from not the VtxDistAdmin
Isolated success case for ensuring storage is updated and RewardRegistered event thrown for each reward
- Rewards should be the size of MaxRewards
Failure case for when VtxDistStatus is not enabled

trigger_vtx_distribution extrinsic:

Failure case for if called from not the VtxDistAdmin
Failure case for if there is no vault account
Isolated success case for ensuring all assets + root are distributed from the fee_vault to the new vault account based on vault balance
- This should ensure the fee_vault does not have any balance left after the transfer
- This should be tested with multiple assets set for AssetPrices (Including XRP and custom assets)
- This extrinsic also needs an event to be thrown

redeem_tokens_from_vault extrinsic:

Failure case for if there is no vault account
Test case for if the total issuance of the VtxAsset is zero, it looks as if it would attempt to divide by zero in this case which would panic. We should not let this happen
Failure case for if the vortex_balance is less than zero
Isolated success case that ensures the correct assets are transferred from the vault account to the signer
- This should also check that the correct amount of the vortex asset is burnt
- This extrinsic should throw an event that should be asserted here

JasonTulp · 2023-11-09T22:15:31Z

pallet/vortex-distribution/src/lib.rs

+			VtxDistStatuses::<T>::mutate(id, |status| {
+				*status = VtxDistStatus::NotEnabled;
+			});
+			// clean ledger when necessary


This appears to be a TODO, is the cleaning logic missing?

JasonTulp · 2023-11-09T22:36:25Z

e2e/common/index.ts

@@ -644,3 +644,22 @@ export const executeForPreviousEvent = async (
    currentInHistory++;
  }
 };
+
+export const loadTestUsers = (userAmount?: number): KeyringPair[] => {
+  const content = fs.readFileSync(path.resolve(__dirname, "./generated_users.txt"), "utf-8");


Why hardcode the generated user list? Could we not instead create random accounts and store them when the test is run?

agreed here; shouldn't be load testing in CI

pallet/vortex-distribution/src/lib.rs

JasonTulp · 2023-11-09T23:42:46Z

pallet/vortex-distribution/src/lib.rs

+			asset_prices: BoundedVec<(AssetId, BalanceOf<T>), T::MaxAssetPrices>,
+			id: T::VtxDistIdentifier,
+		) -> DispatchResultWithPostInfo {
+			for (asset_id, price) in asset_prices {


What happens if the asset_id is the vortex token asset_id? Should we allow the price to be set or fail?

JasonTulp · 2023-11-10T00:40:58Z

pallet/vortex-distribution/src/lib.rs

+		///
+		/// `asset_prices` - List of asset prices
+		/// `id` - The distribution id
+		#[pallet::weight(<T as pallet::Config>::WeightInfo::set_asset_prices())]


nit. ideally the benchmark for setting asset prices should take into account the length of asset_prices

JasonTulp · 2023-11-10T00:44:59Z

pallet/vortex-distribution/src/benchmarking.rs

+		let asset_id = mint_asset::<T>(&fee_vault, &root_vault);
+		assert_ok!(VortexDistribution::<T>::create_vtx_dist(RawOrigin::Root.into(), vortex_dist_id, amount));
+		let balance = <T as pallet_staking::Config>::CurrencyBalance::one();
+		let asset_prices = BoundedVec::try_from(vec![(asset_id, balance.into())]).unwrap();


This benchmark only takes into account one asset set in asset_prices. This is an issue as the majority of reads and writes are determined by the length of AssetPrices. Although it's not possible to determine the number of assets in the benchmark, we should mock a higher number of assets in this benchmark (Say 3-5) so that we can more accurately cover the cost of redeeming multiple assets

pallet/vortex-distribution/src/lib.rs

JasonTulp · 2023-11-10T01:22:12Z

runtime/src/lib.rs

+	type UnsignedInterval = UnsignedInterval;
+	type PayoutBatchSize = PayoutBatchSize;
+	type VtxDistIdentifier = u32;
+	type VtxDistAdminOrigin = EnsureRoot<AccountId>; //TODO: change to proper committee later on


TODO, should this be the Multisig pallet?

JasonTulp · 2023-11-10T01:53:14Z

One more thing that is cause for concern is that the pay_unsigned transaction does not take into account the weight of paying out up to 799 accounts. I attempted to multiply the weight by 799 as the worst case but this exhausted block limits which tells me that this limit is too high.
My suggestion would be that we set the PayoutBatchSize to 100 (20% of the block weight) and measure weight as if 100 accounts were being paid out in the benchmark

surangap · 2023-11-10T02:01:54Z

One more thing that is cause for concern is that the pay_unsigned transaction does not take into account the weight of paying out up to 799 accounts. I attempted to multiply the weight by 799 as the worst case but this exhausted block limits which tells me that this limit is too high. My suggestion would be that we set the PayoutBatchSize to 100 (20% of the block weight) and measure weight as if 100 accounts were being paid out in the benchmark

yes, also the benchmark should take the variability into account. PayoutBatchSize should be taken as the bound when writing benchmarks. A similar benchmark can be found here ->

trn-seed/pallet/futurepass/src/benchmarking.rs

Line 76 in 80bc33c

register_delegate_with_signature {

zees-dev · 2023-11-12T20:51:59Z

rebase over main required
the issues in the CI need to be addressed

zees-dev · 2023-11-12T20:55:47Z

e2e/test/Vortex.test.ts

+    txs = [api.tx.assetsExt.createAsset("test1", "TEST1", 6, 1, alith.address)];
+    await finalizeTx(alith, api.tx.utility.batch(txs));
+
+    TOKEN_ID_2 = await getNextAssetId(api);
+    txs = [api.tx.assetsExt.createAsset("test2", "TEST2", 6, 1, alith.address)];
+    await finalizeTx(alith, api.tx.utility.batch(txs));
+
+    TOKEN_ID_3 = await getNextAssetId(api);
+    txs = [api.tx.assetsExt.createAsset("test3", "TEST3", 6, 1, alith.address)];
+    await finalizeTx(alith, api.tx.utility.batch(txs));
+
+    TOKEN_ID_4 = await getNextAssetId(api);
+    txs = [api.tx.assetsExt.createAsset("test4", "TEST4", 6, 1, alith.address)];
+    await finalizeTx(alith, api.tx.utility.batch(txs));


nit: these are all single transactions that we await for; why batch

zees-dev · 2023-11-12T20:55:58Z

e2e/test/Vortex.test.ts

+    // and would redeem afterwards.
+    // There will be 2 same vortex distributions happening right after.
+    let txs = [api.tx.assets.mint(VORTEX_ID, alith.address, mintAmount)];
+    await finalizeTx(alith, api.tx.utility.batch(txs));


same here; no need to batch

zees-dev · 2023-11-12T20:58:24Z

e2e/test/Vortex.test.ts

+
+    // create vortex distribution
+    const vortexDistributionId1 = await api.query.vortexDistribution.nextVortexId();
+    console.log(`vortexDistributionId1: ${vortexDistributionId1}`);


note: ideally we try to avoid console logs in the tests as it pollutes CI output - making it hard to resolve broken tests in CI

zees-dev · 2023-11-12T21:00:32Z

e2e/hardhat.config.ts

@@ -45,7 +45,7 @@ const config: HardhatUserConfig = {
    apiKey: process.env.ETHERSCAN_API_KEY,
  },
  mocha: {
-    timeout: 120_000, // global tests timeout
+    timeout: 6_000_000, // global tests timeout


the timeout shouldnt be this high; this implies 6000 seconds right - which is 100 minutes.
CI should never be that long.
It would. be best to identify the long test and reduce the timing of it (test on a subset or something).

yeah agreed here, I believe the root cause of this is because they seem want to do a load test inside this CI, which is not ideal. I will ask them to update this e2e folder later today due to the time difference.

yeah, I think the load testing should be separated from the usual CI flow which aims at regression testing.

JasonTulp

lgtm, thanks for addressing all my concerns from the previous review. There are still some minor things that need fixing in follow up PRs but I'm happy to merge this

JasonTulp · 2023-11-12T21:29:49Z

pallet/vortex-distribution/src/benchmarking.rs

+	register_rewards {
+		let vortex_dist_id = NextVortexId::<T>::get();
+		let amount = 1_000_000u32.into();
+		let rewards = BoundedVec::try_from(vec![(account::<T>("test"), 100u32.into())]).unwrap();		assert_ok!(VortexDistribution::<T>::create_vtx_dist(RawOrigin::Root.into(), amount));


Not sure what happened here but looks like these lines have merged into one

Suggested change

let rewards = BoundedVec::try_from(vec![(account::<T>("test"), 100u32.into())]).unwrap(); assert_ok!(VortexDistribution::<T>::create_vtx_dist(RawOrigin::Root.into(), amount));

let rewards = BoundedVec::try_from(vec![(account::<T>("test"), 100u32.into())]).unwrap();

assert_ok!(VortexDistribution::<T>::create_vtx_dist(RawOrigin::Root.into(), amount));

e2e/test/Vortex.test.ts

surangap

good for a merge.

zees-dev reviewed Oct 29, 2023

View reviewed changes

ken-futureverse requested review from surangap and JasonTulp November 1, 2023 00:48

JasonTulp reviewed Nov 1, 2023

View reviewed changes

surangap reviewed Nov 2, 2023

View reviewed changes

pallet/vortex-distribution/src/lib.rs Show resolved Hide resolved

surangap reviewed Nov 2, 2023

View reviewed changes

pallet/vortex-distribution/src/lib.rs Show resolved Hide resolved

surangap reviewed Nov 2, 2023

View reviewed changes

leo-plugdefi force-pushed the feat/vortex-token-distribution branch 2 times, most recently from 2f1f25e to 6f3d254 Compare November 4, 2023 10:57

zees-dev reviewed Nov 7, 2023

View reviewed changes

e2e/common/index.ts Show resolved Hide resolved

zees-dev reviewed Nov 7, 2023

View reviewed changes

evm-precompiles/futurepass/README.md Outdated Show resolved Hide resolved

zees-dev reviewed Nov 7, 2023

View reviewed changes

runtime/src/weights/mod.rs Show resolved Hide resolved

zees-dev reviewed Nov 7, 2023

View reviewed changes

runtime/src/weights/mod.rs Show resolved Hide resolved

zees-dev reviewed Nov 7, 2023

View reviewed changes

runtime/src/constants.rs Outdated Show resolved Hide resolved

zees-dev reviewed Nov 7, 2023

View reviewed changes

runtime/src/lib.rs Outdated Show resolved Hide resolved

leo-plugdefi force-pushed the feat/vortex-token-distribution branch from 6f3d254 to cb647ff Compare November 8, 2023 03:48

surangap reviewed Nov 9, 2023

View reviewed changes

pallet/vortex-distribution/src/lib.rs Show resolved Hide resolved

surangap reviewed Nov 9, 2023

View reviewed changes

zees-dev reviewed Nov 9, 2023

View reviewed changes

surangap reviewed Nov 9, 2023

View reviewed changes

pallet/vortex-distribution/src/lib.rs Show resolved Hide resolved

surangap reviewed Nov 9, 2023

View reviewed changes

runtime/src/weights/pallet_multisig.rs Outdated Show resolved Hide resolved

JasonTulp requested changes Nov 9, 2023

View reviewed changes

JasonTulp reviewed Nov 9, 2023

View reviewed changes

JasonTulp reviewed Nov 10, 2023

View reviewed changes

pallet/vortex-distribution/src/lib.rs Show resolved Hide resolved

JasonTulp reviewed Nov 10, 2023

View reviewed changes

pallet/vortex-distribution/src/lib.rs Show resolved Hide resolved

JasonTulp reviewed Nov 10, 2023

View reviewed changes

leo-plugdefi force-pushed the feat/vortex-token-distribution branch 2 times, most recently from 51528cb to e55d8ed Compare November 12, 2023 11:57

leo-plugdefi and others added 5 commits November 13, 2023 09:55

added vortex pallet

2a9635b

Update benchmarks for pallet-multisig on feat/vortex-token-distribution

d91d421

Update benchmarks for pallet-vortex on feat/vortex-token-distribution

709167b

added runtime configuration for testing

784e0ad

fix github workflow

87caa39

leo-plugdefi force-pushed the feat/vortex-token-distribution branch from 017c8b4 to 87caa39 Compare November 12, 2023 20:55

zees-dev reviewed Nov 12, 2023

View reviewed changes

JasonTulp approved these changes Nov 12, 2023

View reviewed changes

zees-dev reviewed Nov 12, 2023

View reviewed changes

e2e/test/Vortex.test.ts Show resolved Hide resolved

Add * 99 weight + skip e2e test

92e4478

leo-plugdefi changed the title ~~WIP: draft impelementatino of vortex token distribution~~ vortex token distribution Nov 12, 2023

zees-dev mentioned this pull request Nov 12, 2023

Release 4.44.0 #737

Closed

4 tasks

fmt mock

a5fe2f7

zees-dev approved these changes Nov 12, 2023

View reviewed changes

surangap approved these changes Nov 12, 2023

View reviewed changes

JasonTulp merged commit 70a69e6 into main Nov 12, 2023
2 checks passed

JasonTulp deleted the feat/vortex-token-distribution branch November 12, 2023 23:23

		assert_eq!(AssetsExt::balance(usdc, &bob), 500_000);
		assert_eq!(Balances::free_balance(&bob), 500_000);

	let rewards = BoundedVec::try_from(vec![(account::<T>("test"), 100u32.into())]).unwrap(); assert_ok!(VortexDistribution::<T>::create_vtx_dist(RawOrigin::Root.into(), amount));
	let rewards = BoundedVec::try_from(vec![(account::<T>("test"), 100u32.into())]).unwrap();
	assert_ok!(VortexDistribution::<T>::create_vtx_dist(RawOrigin::Root.into(), amount));

vortex token distribution #726

vortex token distribution #726

Conversation

leo-plugdefi commented Oct 27, 2023 • edited by felixshu Loading

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

ken-futureverse commented Nov 1, 2023

JasonTulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surangap Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

leo-plugdefi commented Nov 8, 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

surangap Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonTulp 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

Choose a reason for hiding this comment

JasonTulp commented Nov 10, 2023

surangap commented Nov 10, 2023

zees-dev commented Nov 12, 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

JasonTulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surangap left a comment • edited Loading

Choose a reason for hiding this comment

leo-plugdefi commented Oct 27, 2023 •

edited by felixshu

Loading

surangap Nov 9, 2023 •

edited

Loading

surangap Nov 9, 2023 •

edited

Loading

surangap left a comment •

edited

Loading