Skip to content

Conversation

@dmitrylavrenov
Copy link
Contributor

@dmitrylavrenov dmitrylavrenov commented Aug 16, 2022

Runtime tests for token claims and vesting integration paritytech/substrate#441 .

To do

  • test claim call: claiming without vesting works:
    • ensure the claim is gone from the state after the extrinsic is processed
    • ensure the balance of the target account is properly adjusted
    • ensure that the balance is not locked
    • ensure total issuance did not change
  • test claim call: claiming with vesting works
    • ensure the claim is gone from the state after the extrinsic is processed
    • ensure the balance of the target account is properly adjusted
    • ensure that the balance is locked
    • ensure that the vesting is armed for the given account and matches the parameters (balance amount, schedule, etc)
    • ensure total issuance did not change
  • test unlock call: unlocking partial balance works
    • ensure funds are unlocked
    • ensure rounding works as expected
  • test unlock call: unlocking full balance works
    • ensure funds are unlocked
    • ensure rounding works as expected
  • genesis tests
    • ensure the genesis initialization does not crash at any of the built-in chainspecs
    • ensure the genesis config is properly parsed with the combination of types we have configured in our runtime
  • signed ext tests
    • ensure we don't allow dispatching invalid claim transaction
    • ensure the token claim check signed extension does its job of filtering the the invalid claim transactions, preventing them getting into the transaction pool

@dmitrylavrenov dmitrylavrenov requested a review from MOZGIII August 17, 2022 14:29
@dmitrylavrenov dmitrylavrenov marked this pull request as ready for review August 17, 2022 14:29
Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Looks good so far, but a couple of changes are required.

  • the switch_block helper doesn't look right, we should probably invoke a more realistic block switching or none at all
  • the calls are tests via direct invocation, which doesn't test the signed ext, which we should do; I'll add the corresponding entries to the todo list

@dmitrylavrenov dmitrylavrenov force-pushed the claims-integration-tests branch 2 times, most recently from 7c5b8bc to e12449a Compare August 24, 2022 08:55
@dmitrylavrenov dmitrylavrenov requested a review from MOZGIII August 24, 2022 13:11
Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Great! Now I'll do a more thorough review on this code once again, and either request more changes or we'll be good to merge.

@MOZGIII MOZGIII self-requested a review August 24, 2022 14:19
Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Tests for direct calls need to test for the errors too. Errors have to be tested for:

  • direct calls
  • dispatch validate
  • dispatch pre/post

These three implementations are potentially different, so we need to ensure each of them handles errors (in their own way) correctly.
But still, to clarify, no tests for individual SignedExtensions are needed, only for the combined SignedExtra.

Base automatically changed from claims-integration to master August 28, 2022 22:25
@dmitrylavrenov dmitrylavrenov force-pushed the claims-integration-tests branch from d4d234e to a0fa379 Compare August 30, 2022 16:17
@dmitrylavrenov dmitrylavrenov enabled auto-merge (squash) August 30, 2022 16:59
@MOZGIII MOZGIII mentioned this pull request Aug 30, 2022
@dmitrylavrenov dmitrylavrenov merged commit d07c390 into master Aug 30, 2022
@dmitrylavrenov dmitrylavrenov deleted the claims-integration-tests branch August 30, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants