Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pool Cleanup #652
Pool Cleanup #652
Changes from 3 commits
485f4b4
cc252f2
f5b02a7
bc1859e
1d272cd
f52ab62
e1d9bcb
3b6af5c
1436202
57304a8
c2e0c28
0bd19df
3f4526f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets file an issue to revisit performance of this. Either we:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait aren't we going to be iterating over IBC's module accounts like this? If so, we will be making IBC'd tokens never redeemable. Do we need to care about this, cc @sunnya97 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original issue I think the IBC'd LP tokens are never redeemable for now(also there is no use case of IBCing out LP tokens yet). I could apply the first and the third suggestion now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets alias coin to
userPoolShare
here. I think it makes a lot of subsequent lines easier to readThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can zero share be assured everytime(division factor etc.)?
My only concern is that it might not: considering the fact that I remember that somewhere in the code what we did was iterate through coins and send a part of it every iteration, and at the last iteration, we would send the all the remainings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, I originally thought the total shares are only added/subtracted, not divided; I will check if there are any case of division.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fixed? / checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because shares are just a sdk.Coin, it could be IBC'd out or sent to module logics, and stay under the module account. I am pretty sure if we count all the address held shares and module held shares, it would sum up to GetTotalShares(). Let me add the logic for module accounts to(or, is it already iterated by GetAllBalances?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you investigate if module accounts are iterated over? I think we should definitely exclude certain module accounts, e.g. IBC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iteration do include the module accounts. I will exclude the module account held LP tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, annoyingly, there is no way to distinguish module address and normal address by only using the address itself. We could read all the module address and compare to them each time iterating over the list of address, but at this stage it feels little adhoc. Let me think about some alternatives a bit.