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

Fix: Synchronize position deletion in PositionsByPoolId #151

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

pri-3x
Copy link
Contributor

@pri-3x pri-3x commented Dec 3, 2024

Overview

fix #143
This PR addresses an issue where the Delete method did not synchronize the deleted position in the PositionsByPoolId store.

Changes Made:

  • Updated the RemovePosition method in the store_position.go file to ensure that when a position is deleted from the main store, it is also removed from the corresponding PositionsByPoolId store.
  • Added logic to retrieve the PoolId of the deleted position and delete the position from the pool-specific store.

Impact:

This change ensures that the state of the positions is consistent across all relevant stores, preventing potential issues with data integrity when querying positions by pool ID after deletions.

@@ -85,6 +85,14 @@ func (k Keeper) RemovePosition(ctx context.Context, id uint64) {
storeAdapter := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx))
store := prefix.NewStore(storeAdapter, types.KeyPrefix(types.PositionKey))
store.Delete(GetPositionIDBytes(id))

// Remove from PositionsByPoolId
position, found := k.GetPosition(ctx, id)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this function is executed after store.Delete(GetPositionIDBytes(id)), it would always return [Empty], false.

Shouldn't it be like below?

func (k Keeper) RemovePosition(ctx context.Context, id uint64) {
	storeAdapter := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx))
	store := prefix.NewStore(storeAdapter, types.KeyPrefix(types.PositionKey))
	store.Delete(GetPositionIDBytes(id))

	k.RemovePositionsByPool(ctx, id)
	k.RemovePositionsByAddress(ctx, id)
}
// Remove PositionBy Pool
func RemovePositionsByPool(...) {
  store = prefix.NewStore(storeAdapter, types.PositionByPoolPrefix(poolId))
  iterator := storetypes.KVStorePrefixIterator(store, []byte{})
  defer iterator.Close()

  for ; iterator.Valid(); iterator.Next() {
    id := sdk.BigEndianToUint64(iterator.Value())
    store.Delete(iterator.Value())
  }
}
// Remove PositionsByAddress
func RemovePositionsByAddress(...) {
  // Do similar things
}

@kimurayu45z
Copy link
Contributor

Thank you for submitting a Pull Request, I appreciate it!
Please check the review comment.

@pri-3x
Copy link
Contributor Author

pri-3x commented Dec 3, 2024

Thank you for submitting a Pull Request, I appreciate it! Please check the review comment.

Thanks for the feedback, can you review the changes again?

@kimurayu45z
Copy link
Contributor

Thank you for revision!
I will merge this PR, and I will fix slight remaining parts.

@kimurayu45z kimurayu45z self-requested a review December 3, 2024 16:21
@kimurayu45z kimurayu45z merged commit 456c61a into sunriselayer:main Dec 3, 2024
@kimurayu45z kimurayu45z mentioned this pull request Dec 3, 2024
@pri-3x
Copy link
Contributor Author

pri-3x commented Dec 3, 2024

@kimurayu45z Thankyou :)
Would you be willing to share your email or preferred professional contact method please?

@kimurayu45z
Copy link
Contributor

Contact me in Discord of Sunrise server.
https://discord.com/invite/sunrise

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.

garbage collect deleted store key for sort
2 participants