Skip to content

Conversation

@rajesh-nodle
Copy link
Contributor

Part of #173

HighLights

  • Storage VestingSchedules is moved to bounded vector.
  • Upperlimit for VestingSchedules is Configured via MaxSchedule.
  • Introduce error code MaxScheduleOverflow.
  • UnitTest Extension :: Done
  • Runtime Integration :: Done
  • Storage Migration (try_runtime) :: Done
  • Benchmarking

TODO

Ready for Intake Review

R.Rajeshkumar and others added 30 commits July 13, 2022 00:43
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Co-authored-by: Fredrik Simonsson <simson@thesimson.net>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
R.Rajeshkumar added 2 commits July 20, 2022 14:25
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
@rajesh-nodle
Copy link
Contributor Author

Changes in pallets/allocations/src/migrations.rs is due to fix try-runtime error

Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #642 (cc7136e) into master (e103001) will increase coverage by 0.13%.
The diff coverage is 89.86%.

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
+ Coverage   89.60%   89.74%   +0.13%     
==========================================
  Files          33       33              
  Lines        6087     6238     +151     
==========================================
+ Hits         5454     5598     +144     
- Misses        633      640       +7     
Impacted Files Coverage Δ
runtimes/eden/src/pallets_util.rs 0.00% <ø> (ø)
pallets/grants/src/lib.rs 62.96% <38.23%> (-1.33%) ⬇️
pallets/grants/src/tests.rs 99.20% <99.44%> (-0.80%) ⬇️
pallets/grants/src/benchmarking.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e103001...cc7136e. Read the comment docs.

R.Rajeshkumar added 3 commits July 22, 2022 06:53
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Signed-off-by: R.Rajeshkumar <rajesh@nodle.com>
Comment on lines +151 to +155
log::info!(
"post_upgrade[{:#?}]=> VestingSchedules map count :: [{:#?}]",
line!(),
mapping_count,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could have a better test for post upgrade to actually return Err for some unexpected cases, however it could be an improvement and I will not hold this PR for that. Especially that we have a good test pre-upgrade.

Comment on lines +35 to +37
let releases = Releases::default();
assert_eq!(releases, Releases::V0);
assert_ne!(releases, Releases::V1);
Copy link
Contributor

@aliXsed aliXsed Jul 24, 2022

Choose a reason for hiding this comment

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

I think this test didn't need to be called inside ExtBuilder...execute_with, but not an issue.

Copy link
Contributor

@aliXsed aliXsed left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @rajesh-nodle for taking all the suggestions patiently.

@aliXsed aliXsed merged commit a331f30 into master Jul 25, 2022
@aliXsed aliXsed deleted the shamb0/v3_bounded_grants branch July 25, 2022 00:07
})
.collect::<Vec<_>>()
.try_into()
.expect("Genesis Init Failed Vesting Schedules Overflow");
Copy link
Contributor

Choose a reason for hiding this comment

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

todo check if this is done only in try-runtime and never will make it to the runtime

})
Self::vesting_schedules(who).iter().fold(Zero::zero(), |acc, s| {
acc.checked_add(&s.locked_amount(now)).expect(
"locked amount is a balance and can't be higher than the total balance stored inside the same integer type; qed",
Copy link
Contributor

Choose a reason for hiding this comment

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

todo check if this code is only present in tryruntime sand never will make it into runtime.

if let Ok(new_vesting) = BoundedVec::<VestingScheduleOf<T>, T::MaxSchedule>::try_from(old_vesting) {
<VestingSchedules<T>>::insert(account, new_vesting);
} else {
log::error!(
Copy link
Contributor

Choose a reason for hiding this comment

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

no panic - good.

line!(),
err
)
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Unwrap or else panic will still panic just with an error message.

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.

4 participants