CRT constraints and AMM benchmarks by dobertRowneySr · Pull Request #4750 · Joystream/joystream

dobertRowneySr · 2023-05-05T08:37:41Z

fixes:

max yearly patronage rate
patronage rate computation & approximation updated
patronage tests refactored & updated
add max limit for transfers
adds benchmarks for amm (as Amm benchmark #4481 has lost all changes)

Resources and References:

vercel · 2023-05-05T08:37:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
pioneer-testnet	⬜️ Ignored (Inspect)		May 19, 2023 0:15am

…nd runtime/Cargo.toml

runtime-modules/project-token/src/lib.rs

dobertRowneySr · 2023-05-11T09:01:14Z

runtime-modules/project-token/src/lib.rs

        /// - respective JOY amount transferred from amm treasury account to user account
        /// - event deposited
-        #[weight = 100_000_000] // TODO: adjust weight
-        fn sell_on_amm(origin, token_id: T::TokenId, member_id: T::MemberId, amount: <T as Config>::Balance, deadline: Option<<T as timestamp::Config>::Moment>, slippage_tolerance: Option<(Permill, JoyBalanceOf<T>)>) -> DispatchResult {


This is good, timestamp info can be manipulated by validator node (https://neptunemutual.com/blog/understanding-block-timestamp-manipulation/)

It doesn't look like the deadline was used for any purpose anyway, so it makes sense to drop it.

runtime-modules/project-token/src/lib.rs

runtime-modules/project-token/src/tests/patronage.rs

dobertRowneySr · 2023-05-11T09:33:11Z

runtime/src/lib.rs

    pub const ProjectTokenModuleId: PalletId = PalletId(*b"mo:token"); // module: token
    pub const MaxVestingSchedulesPerAccountPerToken: u32 = 5;
    pub const BlocksPerYear: u32 = 5259600; // 365,25 * 24 * 60 * 60 / 6
+    pub const MaxOutputs: u32 = 256; // TODO(Martin, Ignazio) : find a suitable value


Need a separate issue for:

max patronage rate

min amm slope

runtime-modules/project-token/src/types.rs

dobertRowneySr · 2023-05-11T09:52:37Z

bin/node/src/chain_spec/project_token_config.rs

        min_revenue_split_duration: days!(21),
        min_revenue_split_time_to_start: 0,
        sale_platform_fee: Permill::from_percent(2),
+        max_yearly_patronage_rate: Permill::from_percent(5).into(),


no migration is needed for these parameters as they are new with V2002

runtime-modules/content/src/benchmarks/benchmarking.rs

dobertRowneySr · 2023-05-11T09:55:26Z

runtime-modules/content/src/lib.rs

Ok it appears no (data) migrations are needed here on content pallet

dobertRowneySr · 2023-05-11T10:04:04Z

runtime-modules/project-token/src/tests/patronage.rs

-    let config = GenesisConfigBuilder::new_empty()
-        .with_token_and_owner(token_id, params.build(), owner_id, supply)
-        .build();
+fn claim_patronage_ok_with_correct_credit_accounting_and_more_than_100_percent_supply() {


This also accounts for testing numeric precision, however there are several tests in the common/numerical.rs library for this

dobertRowneySr · 2023-05-11T10:07:10Z

query-node/mappings/src/proposals.ts

+  const specVersion = block.runtimeVersion.specVersion
  const [proposalId, generalProposalParameters, runtimeProposalDetails, proposalThreadId] =
-    new ProposalCreatedEvent_V1001(event).params
+    specVersion >= 2002 ? new ProposalCreatedEvent_V2002(event).params : new ProposalCreatedEvent_V1001(event).params


Should we update the mappings in order to account for the new update max yearly patronage rate proposal?
If so I need to open an issue about this

Will it depends on when we decide to enable creating tokens, or start adding integration tests then yes we would need to update the mappings starting with handling new proposal type in parseProposalDetails()

[Argus] add auth options to elastic logs config

Update librocksdb-sys from v0.6.1+6.28.2 to v0.6.3+6.28.2

mnaamani

I left just an initial high level review, did not go into implementation details yet.

mnaamani · 2023-05-12T07:20:12Z

bin/node/src/chain_spec/project_token_config.rs

        min_revenue_split_time_to_start: 0,
        sale_platform_fee: Permill::from_percent(2),
+        max_yearly_patronage_rate: Permill::from_percent(20).into(), // TODO: update, raising might make benchmarks fail
+        min_amm_slope_parameter: 10, // TODO: update, raising might make benchmarks fail


The default storage value set in project-token decl_storage macro is 15% for max_yearly_patronage_rate and 0 for min_amm_slope_parameter. In a runtime upgrade those defaults will be used because they are now being introduced in nara upgrade. If we are not happy with the defaults, we need to add a migration on_runtime_upgrade.

mnaamani · 2023-05-12T07:28:20Z

query-node/mappings/src/proposals.ts

+  const specVersion = block.runtimeVersion.specVersion
  const [proposalId, generalProposalParameters, runtimeProposalDetails, proposalThreadId] =
-    new ProposalCreatedEvent_V1001(event).params
+    specVersion >= 2002 ? new ProposalCreatedEvent_V2002(event).params : new ProposalCreatedEvent_V1001(event).params


Will it depends on when we decide to enable creating tokens, or start adding integration tests then yes we would need to update the mappings starting with handling new proposal type in parseProposalDetails()

mnaamani · 2023-05-12T07:29:43Z

runtime-modules/common/src/numerical.rs

+    FixedPointNumber, FixedU128, PerThing, Permill, Perquintill,
+};
+
+pub const ORDER: usize = 20;


could you comment what this constant stands for?

mnaamani · 2023-05-15T14:31:30Z

runtime/src/proposals_configuration/testing.rs

+pub(crate) fn update_max_yearly_patronage_rate_proposal() -> ProposalParameters<BlockNumber, Balance>
+{
+    ProposalParameters {
+        voting_period: days!(7),


voting and grace periods for testing,playground need to be much lower. See values for other proposals.

mnaamani · 2023-05-15T14:32:16Z

runtime/src/weights/block_weights.rs


 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2022-10-20 (Y/M/D)
+//! DATE: 2023-05-11 (Y/M/D)


block, extrinsics and rocksdb _weights.rs should not be modified in this PR.

mnaamani · 2023-05-15T14:42:01Z

runtime-modules/content/src/weights.rs

+	// Storage: Token TokenInfoById (r:1 w:1)
+	// Storage: Token MinAmmSlopeParameter (r:1 w:0)
+	// Storage: System Account (r:1 w:1)
+	fn activate_amm() -> Weight {


perhaps we can keep only the new weight functions (and maybe issue_new_token which was modified) in this file and checkout all other changes.
We will do a final weight generation step before nara release.

mnaamani · 2023-05-15T14:53:31Z

runtime-modules/project-token/src/lib.rs

        /// - respective JOY amount transferred from amm treasury account to user account
        /// - event deposited
-        #[weight = 100_000_000] // TODO: adjust weight
-        fn sell_on_amm(origin, token_id: T::TokenId, member_id: T::MemberId, amount: <T as Config>::Balance, deadline: Option<<T as timestamp::Config>::Moment>, slippage_tolerance: Option<(Permill, JoyBalanceOf<T>)>) -> DispatchResult {


It doesn't look like the deadline was used for any purpose anyway, so it makes sense to drop it.

mnaamani · 2023-05-15T15:12:04Z

runtime-modules/content/src/benchmarks/mod.rs

 // Creator tokens
 const MAX_CRT_INITIAL_ALLOCATION_MEMBERS: u32 = 1024;
-const MAX_CRT_ISSUER_TRANSFER_OUTPUTS: u32 = 1024;
+const MAX_CRT_ISSUER_TRANSFER_OUTPUTS: u32 = 256; // same as in Token MaxOuputs


If it is a runtime constant, perhaps it can be read from the pallet instead of re-defining a separate constant and having to remember to keep them in sync?

Argus fix: Cache cleanup not working properly for objects in group `0`

Orion v2 docker setup

mnaamani · 2023-05-17T05:40:23Z

runtime-modules/common/src/numerical.rs

+pub const ORDER: usize = 20;
+
+// does not work with 100% interest
+pub fn natural_log_1_plus_x(interest: Permill) -> Perquintill {


I see this is not used expect internally from one_plus_interest_pow_frac so maybe keep it private?
Similarly for one_plus_interest_pow_frac.

If this function "does not work with 100% interest" would would the output be, and should we guard against invalid input?

mnaamani · 2023-05-17T06:22:14Z

runtime-modules/common/src/numerical.rs

+    result
+}
+
+pub fn one_plus_interest_pow_frac(interest: Permill, exp: Perquintill) -> FixedU128 {


Can you add rust-doc just to show what is being calculated

runtime-modules/project-token/src/benchmarking.rs

mnaamani · 2023-05-17T07:00:03Z

runtime-modules/project-token/src/benchmarking.rs

-
-// Patronage
-const DEFAULT_PATRONAGE: YearlyRate = YearlyRate(Permill::from_percent(1));
+const DEFAULT_SPLIT_ALLOCATION: u32 = 4_000_000; // DEFAULT_REVENUE_SPLIT_RATE * DEFAULT_SPLIT_REVENUE


Why not evaluate it as per commentDEFAULT_REVENUE_SPLIT_RATE * DEFAULT_SPLIT_REVENUE

because DEFAULT_PATRONAGE: Permill and the multiplaction is a non constant operation, so rustc will throw error

mnaamani · 2023-05-17T07:02:58Z

runtime-modules/project-token/src/benchmarking.rs

-            })
-            .collect()
-        );
+        let mut outputs = TransferOutputsOf::<T>::with_bounded_capacity((o + 1) as usize);


by adding + 1 are not going over the MAX_TX_OUTPUTS. range of o starts at 1, so I don't see the need to add +1 if we are concerned calling with_bounded_capacity with args 0..

mnaamani · 2023-05-18T06:55:07Z

runtime-modules/project-token/src/tests/patronage.rs

+        let approx =
+            Token::account_info_by_token_and_member(DEFAULT_TOKEN_ID, DEFAULT_ISSUER_MEMBER_ID)
+                .transferrable::<Test>(System::block_number());
+        let target = 1000000181215750585898368181876;


I'm having a hard time accepting these pre-computed values.

mnaamani · 2023-05-18T07:00:25Z

runtime-modules/project-token/src/types.rs

 use crate::{errors::Error, Config, RepayableBloatBondOf};

+// trait "aliases"
+// `BlockNumber` will be implemented as `u64` in the runtime configuration


The runtime implements BlockNumber as u32, not u64. Is this assumption here problematic?

No, it should not. According to the trait

mnaamani · 2023-05-18T07:03:16Z

runtime-modules/project-token/src/types.rs

            provided_supply: Balance::zero(),
        }
    }
-    pub(crate) fn eval<T: Config>(


This seemed like a better place for the 'eval' implementation., but it was made into a standalone function in the content pallet. What is the reason behind this?

mnaamani · 2023-05-18T07:04:19Z

runtime-modules/project-token/src/types.rs

+impl Add for YearlyRate {
+    type Output = Self;
+    fn add(self, rhs: Self) -> Self::Output {
+        Self(self.0.add(rhs.0))


should we be concerned with overflows?

mnaamani · 2023-05-18T07:15:10Z

runtime/src/utils.rs

 macro_rules! days {
    ($a:expr) => {{
-        hours!(24) * $a
+        ((24 * 60 * 60 * 1000) / ExpectedBlockTime::get()) as u32 * $a


mathematically it looks the same to me, is this implementation more accurate (due to integer maths?)

this was in order to silence clippy, I will revisit

Co-authored-by: Mokhtar Naamani <mokhtar.naamani@gmail.com>

removed the warning to allow array construction and I used iteration utilities for array transversing

mnaamani · 2024-01-26T12:57:20Z

@ignazio-bovo should we close this PR, it was merged into nara if I'm not mistaken?

dobertRowneySr changed the title ~~Nara crt constraints~~ Nara crt constraints and AMM benchmarks May 5, 2023

This was referenced May 5, 2023

Tracker: Runtime CRT work for crt-1 release #4748

Closed

Crt contstraints #4516

Closed

Amm benchmark #4481

Closed

dobertRowneySr added runtime project_token Project tokens nara-network labels May 11, 2023

mnaamani added 2 commits May 11, 2023 11:47

bump joystream-node version to v8.3.1

104e6ef

runtime upgrade test workflow only if changes to runtime/src/lib.rs a…

a191d1c

…nd runtime/Cargo.toml

dobertRowneySr requested review from bedeho and mnaamani May 11, 2023 07:49

dobertRowneySr commented May 11, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

dobertRowneySr commented May 11, 2023

View reviewed changes

mnaamani and others added 7 commits May 11, 2023 15:37

github workflow - fix use of 'git-diff-action'

92153da

runtime upgrade workflow - only test .rs file for changes

77d7380

Fix: Cache cleanup not working for objects in group 0

c66ca38

Merge pull request Joystream#4752 from kdembler/elastic-logs-auth

2801fb8

[Argus] add auth options to elastic logs config

Merge pull request Joystream#4755 from mnaamani/update-librocksdb-sys

efd2940

Update librocksdb-sys from v0.6.1+6.28.2 to v0.6.3+6.28.2

add workerId to argus status response

87857ba

[Argus] allow specifying elastic logs index

a9f09ad

mnaamani reviewed May 15, 2023

View reviewed changes

Lezek123 and others added 3 commits May 15, 2023 17:17

Adjust Orion docker setup: Use separate db service

baca792

Merge pull request Joystream#4757 from Lezek123/argus-fix-cache-drop

36eeb20

Argus fix: Cache cleanup not working properly for objects in group `0`

Merge pull request Joystream#4730 from Lezek123/orion-v2-docker-setup

21251e6

Orion v2 docker setup

mnaamani self-requested a review May 16, 2023 09:30

mnaamani reviewed May 18, 2023

View reviewed changes

Ignazio Bovo and others added 22 commits September 5, 2023 19:55

fix: error

79fe644

fix: TryInto error

567bbec

fix: timestamp not needed

ccec401

fix: macro

2f7a95e

fix: macro days

51118f5

fix: macro

a61ce7a

fix: macro

fe404a5

fix: clippy

ae6393d

fix: testing propsals

cbc0bdf

fix: playground proposal config

0168146

Update runtime-modules/project-token/src/benchmarking.rs

ae9c219

Co-authored-by: Mokhtar Naamani <mokhtar.naamani@gmail.com>

fix: rename

f013811

fix: replace magic numbers in patronage

eaebad9

fix: address changes

a637f52

fix: amm test

e1535bf

fix: make eval a method

9fe6e76

fix: clippy useless conversion

a52af08

fix: refactor test constants

a46e709

fix: benchmarks

2777caf

fix: content benchmarks

009e01d

fix: 🚨 slice indexing warning by clippy

f7d3fbc

removed the warning to allow array construction and I used iteration utilities for array transversing

fix: 🚨 post rebase fixes

28e77ab

dobertRowneySr force-pushed the nara_crt_constraints branch from b84cea5 to 28e77ab Compare September 5, 2023 17:57

bedeho added crt-v1 and removed nara-network labels Sep 13, 2023

bedeho changed the title ~~Nara crt constraints and AMM benchmarks~~ CRT constraints and AMM benchmarks Sep 13, 2023

bedeho added crt_release_placeholder Placeholder to whatever release has CRT and removed crt-v1 labels Sep 13, 2023

dobertRowneySr closed this Mar 14, 2024

Conversation

dobertRowneySr commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resources and References:

Uh oh!

vercel bot commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dobertRowneySr May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dobertRowneySr May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr commented May 5, 2023 •

edited

Loading

vercel bot commented May 5, 2023 •

edited

Loading

dobertRowneySr May 11, 2023 •

edited

Loading

dobertRowneySr May 11, 2023 •

edited

Loading

dobertRowneySr May 11, 2023 •

edited

Loading

dobertRowneySr May 18, 2023 •

edited

Loading