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

CalculateSpotPrice should be fee-less #54

Closed
sunnya97 opened this issue Mar 31, 2021 · 3 comments · Fixed by #177
Closed

CalculateSpotPrice should be fee-less #54

sunnya97 opened this issue Mar 31, 2021 · 3 comments · Fixed by #177
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:code-hygiene

Comments

@sunnya97
Copy link
Collaborator

Currently we have two functions: CalculateSpotPrice and CalculateSpotPriceSansFee.

I know, this is using the naming used in the Balancer code, but I think this nomenclature of SpotPrice including fees is quite non-standard in industry and we should it correctly. (Even the balancer docs, define spot price as not including fees: https://docs.balancer.finance/core-concepts/protocol/index#spot-price).

I'd recommend switching:

  • CalculateSpotPriceSansFee -> CalculateSpotPrice
  • CalculateSpotPrice -> CalculateSpotPriceWithFee.
@ValarDragon
Copy link
Member

Thats whats already done in this branch https://github.com/osmosis-labs/osmosis/tree/gamm_updates

@sunnya97
Copy link
Collaborator Author

sunnya97 commented Apr 1, 2021

Closed by 4a5ed58

@sunnya97 sunnya97 closed this as completed Apr 1, 2021
@sunnya97 sunnya97 reopened this Apr 30, 2021
@sunnya97
Copy link
Collaborator Author

While the lower level unexposed functions were switched, the exposed keeper functions are still CalculateSpotPriceSansFee and CalculateSpotPrice.

See: https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/keeper/swap.go#L238

These should be fixed as well.

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:code-hygiene
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants