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

modify: fix formula bug #410

Merged
merged 5 commits into from
Mar 9, 2023
Merged

modify: fix formula bug #410

merged 5 commits into from
Mar 9, 2023

Conversation

mkXultra
Copy link
Contributor

@mkXultra mkXultra commented Mar 9, 2023

No description provided.

@mkXultra mkXultra requested a review from kimurayu45z March 9, 2023 02:35
@mkXultra mkXultra self-assigned this Mar 9, 2023
func NormalToMicroDenom(amount sdk.Dec) math.Int {
return amount.Mul(sdk.MustNewDecFromStr("1000000")).TruncateInt()
}

func (m PerpetualFuturesPosition) CalcReturningAmountAtClose(closedRate sdk.Dec) (returningAmount math.Int, lossToLP math.Int) {
func (m PerpetualFuturesPosition) CalcReturningAmountAtClose(baseUSDRate, quoteUSDRate sdk.Dec) (returningAmount math.Int, lossToLP math.Int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

全体的に、xxxUSDRateではなくxxxMetricsRateのほうが良いと思います。
一応USDというのはハードコーディングされてるわけではなくてParamsでmetricsのtickerが"USD"だと指定されているだけなので。

Copy link
Contributor Author

@mkXultra mkXultra Mar 9, 2023

Choose a reason for hiding this comment

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

はい、それにしたいです
ただ、debugするときに
sdk.decで引き釣りまわすと、これは何のrateかわからなくなることが多々あります
MetricsRateTypeを作って
{
MetricsUnit: denom
rate sdk.Dec
}
を使いたいです

Copy link
Contributor Author

Choose a reason for hiding this comment

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

↑を書き換えるタイミングで変数名を直すのはどうでしょうか?
POC開始したらすぐに直す予定です

}

// func (m PerpetualFuturesPosition) RequiredMarginInMetrics(requiredMarginInQuote, quoteUSDRate sdk.Dec) sdk.Dec {
func (m PerpetualFuturesPosition) RequiredMarginInMetrics(baseUSDRate, quoteUSDRate sdk.Dec) sdk.Dec {
Copy link
Contributor

Choose a reason for hiding this comment

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

ここではmarginMetricsRateひとつだけを引数にうけとるようにして、

MarginMetricsRate(baseMetricsRate, quoteMetricsRate sdk.Dec) sdk.Dec
みたいな関数つくってそこでif分岐させるのはどうでしょうか。将来的にはbaseまたはquote以外のdenomもmarginにいれられるようにする拡張というのもありえなくはないので。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらもPOC開始後対応でよいですか?
Issueは作っておきます

@mkXultra mkXultra merged commit 66fc70b into feat/derivatives Mar 9, 2023
@mkXultra mkXultra deleted the poc-hotfix branch March 9, 2023 05:33
@Senna46 Senna46 added the enhancement New feature or request label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants