Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add x/services simulation tests #162

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

manu0466
Copy link
Contributor

@manu0466 manu0466 commented Nov 13, 2024

Description

Depends-On: #161
Closes: MILK-130


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

return false, nil
},
)
if err != nil {
panic(err)
}

broken := count != expected
broken := elements > 0 && count != expected
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the simulation test starts, this invariant fails due to the presence of services while PoolHistoricalRewards is empty. I can't figure out how to resolve this issue properly. Any tips on how to address this would be greatly appreciated, @hallazzang.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this logic is incorrect: we should check

  • the targets of pool delegations vs the pool historical rewards
  • the targets of service delegation vs the service historical rewards
  • the targets of operator delegations vs the operators historical rewards

But instead everything is checked against the pool historical rewards. Why is this the case @hallazzang?

Copy link
Contributor

@RiccardoM RiccardoM Nov 14, 2024

Choose a reason for hiding this comment

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

Also, if we need to make sure that there's an entry for each target, we should add a hook for AfterServiceCreated, AfterPoolCreated and AfterOperatorCreated to make sure that we create the first entry for each one of them there. Otherwise, inside a genesis file having some services but not yet delegations, this invariant might fail.

This is the same thing that the Cosmos SDK does:

func (h Hooks) AfterValidatorCreated(ctx context.Context, valAddr sdk.ValAddress) error {
	val, err := h.k.stakingKeeper.Validator(ctx, valAddr)
	if err != nil {
		return err
	}
	return h.k.initializeValidator(ctx, val)
}

I think we should have the same logic. Let me take care of adding this and fixing the invariant as well as adding unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

But instead everything is checked against the pool historical rewards. Why is this the case @hallazzang?

Sorry guys this absolutely seems a bug 😅

Otherwise, inside a genesis file having some services but not yet delegations, this invariant might fail.

Oh that's also true. I don't know why the invariant wasn't broken already? FYI, in our code, we're initializing each delegation target when the first delegation is being created.

Copy link
Contributor

@RiccardoM RiccardoM Nov 14, 2024

Choose a reason for hiding this comment

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

I have now completely changed how this works. Instead of initializing the delegation target when a delegation is created, I have added some hooks to the x/pools, x/operators and x/services modules to make sure we initialize the x/rewards data when these are created.

I have also added some hooks to make sure we clear the store data when a service/operator is deleted (it was missing as well).

Finally, I fixed the invariant and added the missing unit tests to make sure it breaks only when expected.

@manu0466 manu0466 changed the title test: x/services simulation tests test: add x/services simulation tests Nov 13, 2024
@manu0466 manu0466 marked this pull request as ready for review November 13, 2024 15:52
@manu0466 manu0466 marked this pull request as draft November 13, 2024 17:36
Base automatically changed from riccardom/swtich-to-l1 to main November 14, 2024 01:11
@manu0466 manu0466 force-pushed the manuel/services-simulation-tests branch from 04e90b0 to 54bd043 Compare November 14, 2024 08:49
@manu0466 manu0466 marked this pull request as ready for review November 14, 2024 08:58
@RiccardoM RiccardoM merged commit 9612421 into main Nov 15, 2024
17 checks passed
@RiccardoM RiccardoM deleted the manuel/services-simulation-tests branch November 15, 2024 00:08
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.

3 participants