-
Notifications
You must be signed in to change notification settings - Fork 820
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
Futures order position tracking & FTX scaled collateral calculation #868
Conversation
Codecov Report
@@ Coverage Diff @@
## master #868 +/- ##
==========================================
+ Coverage 44.67% 45.00% +0.32%
==========================================
Files 313 314 +1
Lines 79926 81214 +1288
==========================================
+ Hits 35707 36549 +842
- Misses 38762 39122 +360
- Partials 5457 5543 +86
Continue to review full report at Codecov.
|
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.
This is quite a loaded PR, appreciate you pulled it out from the main work you are doing. Looks really good so far, Still need to do more tests after 3 coffees. 🚀
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.
Thanks for addressing all the change requests.
Position tracking testing:
go run ./... --timeout=5m getfuturesposition --exchange="ftx" --asset="futures" --pair="BTC-perp"
{
"totalOrders": 4,
"totalRealisedPNL": "-0.2770445625",
"positions": [
{
"status": "CLOSED",
"unrealisedPNL": "0",
"realisedPNL": "-0.1481740275",
"openingDate": "2021-10-25 23:05:28 +0000",
"closingDate": "2021-10-25 23:06:48 +0000"
},
{
"status": "CLOSED",
"unrealisedPNL": "0",
"realisedPNL": "-0.128870535",
"openingDate": "2021-10-25 23:20:30 +0000",
"closingDate": "2021-10-25 23:20:37 +0000"
}
]
}
Can confirm position return calc from front end.
Would be good in future to see a percentage increase or decrease as well.
Collateral calculation testing:
attempt 1 offline:
go run ./... --timeout=5m getcollateral --exchange="ftx" --asset="spot" --i --c
{
"totalCollateral": "1.07902616",
"currencyBreakdown": [
{
"currency": "USD",
"scaledCollateral": "1.07902616"
}
]
}
another attempt same account:
go run ./... --timeout=5m getcollateral --exchange="ftx" --asset="spot" --i --c
{
"totalCollateral": "4007.3620206",
"currencyBreakdown": [
{
"currency": "BTC",
"scaledCollateral": "0",
"scaledToCurrency": "USD"
},
{
"currency": "OKB",
"scaledCollateral": "0",
"scaledToCurrency": "USD"
},
{
"currency": "BNB",
"scaledCollateral": "0",
"scaledToCurrency": "USD"
},
{
"currency": "USD",
"scaledCollateral": "4007.3620206"
},
{
"currency": "XRP",
"scaledCollateral": "0",
"scaledToCurrency": "USD"
},
{
"currency": "SRM",
"scaledCollateral": "0",
"scaledToCurrency": "USD"
},
{
"currency": "FTT",
"scaledCollateral": "0",
"scaledToCurrency": "USD"
}
]
}
Don't know why it takes a couple requests to populate it all just yet.
Also: When we designate a sub-account via config we cannot utilize getcollateral for main or other sub-accounts, which for now is not a big issue.
From our discussions, it may have been a result of you using a subaccount, but this isn't something I have been able to replicate. If it comes up again, please provide steps to recreate. The RPCServer now returns a subaccount name on both
or
|
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.
Thanks for those changes, just one thing I found when getting collateral. And a pedantic suggestion. 👨🎤
Okay, I think I've done it this time 😄 I swear! I've made it more clear and the valuations are more accurate
It also works with SRM_LOCKED:
|
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.
Collateral looks good and the added lovely display currency is a nice touch! Good stuff! tACK.
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.
I can see that futures UPNL and online collateral calculations are matching up with the front-end 🎉 We're still missing the "additional collateral used" breackdown field per spot margin borrowed item, but that's something I have a ticket with FTX open ATM.
The locked_in_stakes value shows the serum amount in USD, but should be SRM amount * current SRM market price
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.
Recent changes look good and the collateral with the front end matches for me. Online compared to offline is a $1 difference in avail collateral 👍.
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.
Thanks for making those additional changes! Collateral matches FTX front-end and also same with the futures PNL on an account with staking, margin borrowing and free assets 🎉 Only have some minor nits left
cmd/gctcli/commands.go
Outdated
var overwrite bool | ||
if c.IsSet("overwrite") { | ||
overwrite = c.Bool("overwrite") | ||
} else if c.Args().Get(2) != "" { |
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.
} else if c.Args().Get(2) != "" { | |
} else if c.Args().Get(8) != "" { |
cmd/gctcli/commands.go
Outdated
var verbose bool | ||
if c.IsSet("verbose") { | ||
verbose = c.Bool("verbose") | ||
} else if c.Args().Get(6) != "" { |
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.
Do you mean 7?
cmd/gctcli/commands.go
Outdated
} | ||
|
||
var subAccount string | ||
if c.IsSet("subaccount") { |
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.
Subaccount is last in the ArgsUsage blurb, but second last here
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.
tACK, nice work!
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.
This is pretty much a tACK; just one thing I noticed:
Offline calculations have approx_fair_market_value as 0 for USD.
{
"currency": "USD",
"total_funds": "4018.00442214 USD",
"available_for_use_as_collateral": "0.03206414 USD",
"approx_fair_market_value": "0 USD",
"weighting": "1",
"collateral_contribution": "0.03206414 USD",
"scaled_to_currency": "USD",
"funds_in_use": "4017.972358 USD"
},
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.
ChangesACK.
PR Description
This PR is based on the following branch, minus any backtester changes here.
As getting futures support working for the backtester requires many changes, I thought I'd extract some reviewable parts of my work. This PR does the following:
getfuturesposition
(note: mocking my sweet PNL is banned)getcollateral
These features have been implemented for FTX as that's what I'm currently targeting for backtesting futures work
Things to consider
overwrite
when requesting position data via GRPC. It will clear any existing data for the exchange+currencypair+assetType of change
How has this been tested
I've added tests to all new functions as well as added some additional coverage where it came up.
Checklist