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 for RemoveZoneAndAssociatedRecords #678

Conversation

DongLieu
Copy link
Contributor

@DongLieu DongLieu commented Oct 9, 2023

1. Summary

Fixes #617

2.Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

3. Implementation details

4. How to test/use

5. Checklist

  • Does the Readme need to be updated?

6. Limitations (optional)

7. Future Work (optional)

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #678 (e120b08) into main (963ffa9) will increase coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #678      +/-   ##
==========================================
+ Coverage   14.39%   14.44%   +0.05%     
==========================================
  Files         243      243              
  Lines       63127    63133       +6     
==========================================
+ Hits         9089     9122      +33     
+ Misses      53286    53255      -31     
- Partials      752      756       +4     
Flag Coverage Δ
unittests 14.44% <66.66%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
x/interchainstaking/keeper/validators.go 93.15% <ø> (ø)
x/interchainstaking/keeper/zones.go 62.11% <66.66%> (+7.12%) ⬆️

@joe-bowman
Copy link
Contributor

Can we also check for validators / receipts / withdrawal records etc. by zone to ensure that everything that should have been cleaned up has been? thankjs,

@DongLieu
Copy link
Contributor Author

Ok

@DongLieu
Copy link
Contributor Author

DongLieu commented Oct 25, 2023

@joe-bowman
Cannot check:

  • delegation records: because the zone has been deleted

  • performance delegation records: because the zone has been deleted

  • validators: In the RemoveZoneAndAssociatedRecords function do not remove Validator. Set add validators to create related

Additional:
In GetReceipt, I think we should pass by value: 'chainID' and 'TxHash' instead of the current 'key' .PR:#735

@joe-bowman
Copy link
Contributor

Validators should definitely be removed if a zone is removed. Can you make a fix for this, please?

Validator storage changed in v1.4 and the deletion of validators for a given zone was almost certainly overlooked.

@joe-bowman
Copy link
Contributor

joe-bowman commented Oct 26, 2023

  • delegation records: because the zone has been deleted
  • performance delegation records: because the zone has been deleted

Also probably a good time to refactor:

k.IterateAllDelegations(ctx, zone, func(delegation types.Delegation) (stop bool) {
k.IterateAllPerformanceDelegations(ctx, zone, func(delegation types.Delegation) (stop bool) {
k.IterateZoneReceipts(ctx, zone, func(index int64, receiptInfo types.Receipt) (stop bool) {

to take chainID instead of zone (all three methods only make use of zone.ChainID internally anyway!) but allows us to assert the removal properly.

This also ensures the signatures are consistent with the other types (withdrawals, unbonding and redelegation records), and allows us to remove the unnecessary IterateZones call in the function being tested!

@DongLieu
Copy link
Contributor Author

Okay,
Do I create a new PR to do this, or do it in this PR?

@DongLieu
Copy link
Contributor Author

For checks delegation and performance delegation records, I need to wait for the pull #748 to be merged

@DongLieu
Copy link
Contributor Author

I need to merge this PR #758 first, please review it

@DongLieu
Copy link
Contributor Author

DongLieu commented Nov 1, 2023

Ok, done. @ThanhNhann @joe-bowman

Copy link
Contributor

@anhductn2001 anhductn2001 left a comment

Choose a reason for hiding this comment

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

Nice works @DongLieu !

Copy link
Contributor

@joe-bowman joe-bowman left a comment

Choose a reason for hiding this comment

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

lgtm

@joe-bowman joe-bowman merged commit 69eb145 into quicksilver-zone:main Nov 7, 2023
11 checks passed
@joe-bowman joe-bowman deleted the dong/TestRemoveZoneAndAssociatedRecords branch November 7, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants