Adding `try_state` hook(tips & vesting) #13515

Doordashcon · 2023-03-02T16:00:14Z

part of paritytech/polkadot-sdk#239
polkadot address: 12zsKEDVcHpKEWb99iFt3xrTCQQXZMu477nJQsTBBrof5k2h

frame/society/src/mock.rs

frame/tips/src/lib.rs

frame/vesting/src/lib.rs

kianenigma

On the right track, but needs a bit more work.

kianenigma · 2023-03-16T12:34:56Z

Would be good if you update the PR title to say which pallets it includes.

Doordashcon · 2023-04-18T07:18:25Z

I have ran the try-state expectations of Tips and Vesting pallet against the current Polkadot state, please review :)

Doordashcon · 2023-04-18T07:24:34Z

liamaharon · 2023-04-21T08:28:04Z

frame/vesting/src/mock.rs

+		ext.execute_with(|| {
+			System::set_block_number(1);
+			Vesting::do_try_state().unwrap();
+		});


I think this doesn't really test much because it runs before there's any state set up to test?

I suggest that you

test that the try-state hooks will detect the issues that they're supposed to, check out how that's done here: https://github.com/paritytech/substrate/blob/master/frame/bags-list/src/list/tests.rs#L353-L378 (also do this with tips)

pepper assert_ok!(vesting::do_try_state());s into existing tests, like you did with tips

But i still think it's all the same and there's no need to pepper existing tests, the current setup in vesting tests the genesis state.

Let me know if I'm missing something, but I think we should test all functionality not just the genesis state?

Like bags-list there should be tests ensuring that the try-state hooks actually detect the issues that they're supposed to: https://github.com/paritytech/substrate/blob/master/frame/bags-list/src/list/tests.rs#L353-L378

Hello @liamaharon , I have made updates to this. Please review

Please add tests ensuring that the try-state hooks detects the issues they are intended to. They’d have picked up a bug in the assert (see other comment)

frame/vesting/src/lib.rs

liamaharon · 2023-04-21T08:31:03Z

frame/vesting/src/lib.rs

+			let mut total_per_block: BalanceOf<T> = Zero::zero();
+			let mut total_locked_now: BalanceOf<T> = Zero::zero();
+			for info in vesting.iter() {
+				// get the number of remaining vesting blocks<>balance


I don't understand this comment

the number of blocks remaining are converted to balance

Please update this comment with this clarification

frame/vesting/src/lib.rs

liamaharon

I don't mean to sound harsh, but this PR was not ready to be re-reviewed.

The core logic of the vesting try-state hook is broken, there’s an ignored suggestion from the last review which would have uncovered that issue, and comments had not been updated to reflect code changes.

Please only request review after you have addressed comments from the previous review and thoroughly checked your logic, code and comments.

I suggest that you step back from the code for a minute, think about the invariant/s you are testing, and thoughtfully plan the logic that that will test these invariants, writing it down in a plain English doc comment. Then as you codify that logic, write tests to ensure that the code is working as you intended, detecting failure cases as well as letting through success cases.

liamaharon · 2023-05-16T07:55:47Z

frame/vesting/src/lib.rs

+			let one_extra = total_to_vest.saturating_sub(total_locked);
+			assert_eq!(total_to_vest, total_locked.saturating_add(one_extra));


So what this is checking is:

if one_extra = total_to_vest - total_locked does total_to_vest = one_extra + total_locked?

or more abstractly

if x = y - z, then does y = x + z?

of course it will always be true, you're asserting that 1 == 1.

liamaharon · 2023-05-16T07:59:33Z

frame/vesting/src/lib.rs

+			let mut total_per_block: BalanceOf<T> = Zero::zero();
+			let mut total_locked_now: BalanceOf<T> = Zero::zero();
+			for info in vesting.iter() {
+				// get the number of remaining vesting blocks<>balance


Please update this comment with this clarification

liamaharon · 2023-05-16T08:00:46Z

frame/vesting/src/lib.rs

+				let amount_left: BalanceOf<T> =
+					info.ending_block_as_balance::<T::BlockNumberToBalance>();
+				total_to_vest += info.per_block().saturating_mul(amount_left);
+				// get the total block<>balance still locked.


Could you please replace this block<>balance shorthand throughout the comments with just plain English

liamaharon · 2023-05-16T08:01:19Z

frame/vesting/src/lib.rs

+	/// * The `per_block` amount left to be claimed is equal to the
+	/// total balance currently locked.


Outdated variable name in comment

liamaharon · 2023-05-16T08:01:52Z

frame/vesting/src/lib.rs

+	/// *`total_to_vest` the total amount left to vest over all remaining blocks.
+	/// *`total_locked` the total amount locked.
+	///
+	/// * `one_extra` accounts for the extra block that
+	/// will be added to the vesting schedule if `per_block` is bigger than `locked`.


Please add documentation for variables the line above where the variable is declared, rather than in the doc comment

liamaharon · 2023-05-16T08:05:18Z

frame/vesting/src/mock.rs

+		ext.execute_with(|| {
+			System::set_block_number(1);
+			Vesting::do_try_state().unwrap();
+		});


Please add tests ensuring that the try-state hooks detects the issues they are intended to. They’d have picked up a bug in the assert (see other comment)

Doordashcon · 2023-07-25T01:37:29Z

Hello @liamaharon, if you could spare some time...

I am currently trying to denote where the logic is that updates the VestingInfo as block number increases(i.e bob has a vesting info of { locked: 256, per_block: 12, starting_block: 10}).

Time(moment) passes and now we are at block 11, locked gets updated to reflect this.

This is the logic I am searching for, the one that knows to execute the update.

Doordashcon added 2 commits March 2, 2023 16:49

vesting

fcaf225

Tips

70e6c00

xlc reviewed Mar 11, 2023

View reviewed changes

frame/society/src/mock.rs Outdated Show resolved Hide resolved

kianenigma added the A0-please_review Pull request needs code review. label Mar 16, 2023

kianenigma reviewed Mar 16, 2023

View reviewed changes

frame/tips/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Mar 16, 2023

View reviewed changes

frame/vesting/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Mar 16, 2023

View reviewed changes

Doordashcon changed the title ~~Adding try_state hook~~ Adding try_state hook(vesting, tips & sudo) Mar 17, 2023

Doordashcon changed the title ~~Adding try_state hook(vesting, tips & sudo)~~ Adding try_state hook(vesting & tips) Mar 18, 2023

Doordashcon changed the title ~~Adding try_state hook(vesting & tips)~~ Adding try_state hook(tips & vesting) Mar 20, 2023

Doordashcon added 3 commits March 20, 2023 07:06

documentation, revert society

7f01672

revert society

34800d8

cargo fmt

e3194a1

Doordashcon marked this pull request as ready for review March 20, 2023 17:15

Doordashcon added 3 commits April 6, 2023 22:27

attribute fix

bed7f69

Merge branch 'paritytech:master' into ddc-TS-1

4b30985

Merge branch 'paritytech:master' into ddc-TS-1

2d742fa

Doordashcon requested a review from kianenigma April 18, 2023 07:25

liamaharon reviewed Apr 21, 2023

View reviewed changes

liamaharon added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 21, 2023

improve vesting docs & Tips try-state

786f375

Doordashcon requested a review from liamaharon May 10, 2023 07:37

pepper conditional

d9e0f3a

Doordashcon requested a review from a team May 15, 2023 15:27

liamaharon suggested changes May 16, 2023

View reviewed changes

paritytech-ci requested a review from a team May 16, 2023 20:08

kianenigma mentioned this pull request May 22, 2023

Tracking issue for adding try_state hook to all pallets. paritytech/polkadot-sdk#239

Open

62 tasks

juangirini assigned Doordashcon May 22, 2023

juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jun 12, 2023

Merge branch 'paritytech:master' into ddc-TS-1

834a28c

Doordashcon requested a review from a team June 28, 2023 07:33

Doordashcon added 7 commits July 30, 2023 22:56

WIP

42410fe

vesting docs, tips result

6a5a764

Merge branch 'paritytech:master' into ddc-TS-1

3c539a9

oversight print macro

8eb23d0

feature flag SaturatedConversion

dc502ee

fully Q

52d40d4

ensure

fda9110

Doordashcon requested a review from ggwpez August 8, 2023 12:14

slit vesting logic into seperate fn

b235a18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `try_state` hook(tips & vesting) #13515

Adding `try_state` hook(tips & vesting) #13515

Doordashcon commented Mar 2, 2023 •

edited

Loading

kianenigma left a comment

kianenigma commented Mar 16, 2023

Doordashcon commented Apr 18, 2023

Doordashcon commented Apr 18, 2023

liamaharon Apr 21, 2023 •

edited

Loading

Doordashcon May 4, 2023 •

edited

Loading

liamaharon May 11, 2023

Doordashcon May 15, 2023

liamaharon May 16, 2023 •

edited

Loading

liamaharon Apr 21, 2023 •

edited

Loading

Doordashcon May 4, 2023

liamaharon May 16, 2023

liamaharon left a comment •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

Doordashcon commented Jul 25, 2023

		let one_extra = total_to_vest.saturating_sub(total_locked);
		assert_eq!(total_to_vest, total_locked.saturating_add(one_extra));

		/// * The `per_block` amount left to be claimed is equal to the
		/// total balance currently locked.

Adding try_state hook(tips & vesting) #13515

Are you sure you want to change the base?

Adding try_state hook(tips & vesting) #13515

Conversation

Doordashcon commented Mar 2, 2023 • edited Loading

kianenigma left a comment

Choose a reason for hiding this comment

kianenigma commented Mar 16, 2023

Doordashcon commented Apr 18, 2023

Doordashcon commented Apr 18, 2023

liamaharon Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Doordashcon May 4, 2023 • edited Loading

Choose a reason for hiding this comment

liamaharon May 11, 2023

Choose a reason for hiding this comment

Doordashcon May 15, 2023

Choose a reason for hiding this comment

liamaharon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

liamaharon Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Doordashcon May 4, 2023

Choose a reason for hiding this comment

liamaharon May 16, 2023

Choose a reason for hiding this comment

liamaharon left a comment • edited Loading

Choose a reason for hiding this comment

liamaharon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

liamaharon May 16, 2023

Choose a reason for hiding this comment

liamaharon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

liamaharon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

liamaharon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

liamaharon May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Doordashcon commented Jul 25, 2023

Adding `try_state` hook(tips & vesting) #13515

Adding `try_state` hook(tips & vesting) #13515

Doordashcon commented Mar 2, 2023 •

edited

Loading

liamaharon Apr 21, 2023 •

edited

Loading

Doordashcon May 4, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon Apr 21, 2023 •

edited

Loading

liamaharon left a comment •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading

liamaharon May 16, 2023 •

edited

Loading