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

[x/gamm][Testing] balancer/CalcJoinPoolShares #1372

Closed
Tracked by #1452
p0mvn opened this issue Apr 28, 2022 · 1 comment · Fixed by #1430
Closed
Tracked by #1452

[x/gamm][Testing] balancer/CalcJoinPoolShares #1372

p0mvn opened this issue Apr 28, 2022 · 1 comment · Fixed by #1430
Assignees
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:task ⚙️ A task belongs to a story T:tests

Comments

@p0mvn
Copy link
Member

p0mvn commented Apr 28, 2022

Background

We have observed rounding errors in balancer post-refactor (#1307). There are several balancer pool methods being called from the function in #1307 under the hood that are not unit-tested.

As a result, it is non-trivial to identify the source of the problem. All methods should be covered by unit tests. One of such untested methods is CalcJoinPoolShares

On initial investigation, this method does many things, violating the single responsibility principle in SOLID. As a result, it is difficult to reason about and test.

The person taking on this issue should investigate if it is worth refactoring this method to extract a few others to be called under the hood to mitigate the violation. If any, all newly extracted methods should be unit tested, including CalcJoinPoolShares itself.

Acceptance Criteria

  • investigate the need for extracting several methods from CalcJoinPoolShares to ease unit testing and improve readability
  • ensure all logic in CalcJoinPoolShares and newly extracted functions/methods, if any, are covered by unit tests
  • edge cases are well thought out
  • all old and new unit tests pass
@daniel-farina daniel-farina moved this to 🔍 Needs Review in Osmosis Chain Development Apr 28, 2022
@p0mvn p0mvn added T:tests C:x/gamm Changes, features and bugs related to the gamm module. labels Apr 28, 2022
@p0mvn
Copy link
Member Author

p0mvn commented Apr 28, 2022

@alexanderbez would you be interested in working on this? Thanks for offering to help with the tests

@alexanderbez alexanderbez self-assigned this Apr 28, 2022
@p0mvn p0mvn changed the title refactor and test: balancer/CalcJoinPoolShares [x/gamm][Testing] balancer/CalcJoinPoolShares May 9, 2022
@p0mvn p0mvn added the T:task ⚙️ A task belongs to a story label May 9, 2022
@p0mvn p0mvn mentioned this issue May 9, 2022
7 tasks
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:task ⚙️ A task belongs to a story T:tests
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants