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

DFI2203 Futures #1155

Merged
merged 12 commits into from
Mar 28, 2022
Merged

DFI2203 Futures #1155

merged 12 commits into from
Mar 28, 2022

Conversation

Bushstar
Copy link
Member

No description provided.

bvbfan
bvbfan previously approved these changes Mar 22, 2022
src/masternodes/govvariables/attributes.cpp Show resolved Hide resolved
src/masternodes/rpc_accounts.cpp Show resolved Hide resolved
src/masternodes/rpc_accounts.cpp Outdated Show resolved Hide resolved
src/masternodes/rpc_oracles.cpp Outdated Show resolved Hide resolved
@kuegi
Copy link
Contributor

kuegi commented Mar 23, 2022

Sorry for being late to the party/review, but why call it dfip2203 everywhere in the code? wouldn't a more descriptive name like "futures" be better? dfip2202 was a specific smart contract for this purpose, so I get the name. But here we have a nice functionality that can be named instead of a "generic" dfip number.

@kuegi
Copy link
Contributor

kuegi commented Mar 23, 2022

sorry if i misunderstand it completly, but from the code, the logic feels totally off.

What I understand from the code how it works:

  • Users can create a future contract basically saying I will buy TSLA for X DUSD at the next settlement but without specifying a current price. So when I do this contract, I need to guess what the futures price will be, (up to a week from now). Seems like there is no reason whatsoever to do that more than a few blocks before the settlement (when I know the current price). (In combination with the next point, this kinda "makes sense", but in the way a future should work, this makes no sense)
  • The price where the future settles seems to get set on the previous futures block (to the active oracle price +- rewardPct). So you would say In one week time, the future will settle for the current price +5% right? But this contradicts the whole idea behind the futures. They should settle at the current active oracle price (+- 5%) at the time of settlement, not from the active price 1 week before the settlement. Otherwise this whole thing will lock the price of a dToken within a 5% range of the current price, for the next week.

Shouldn't the future be a pair that can be traded normally and its just fixed that all positions will be settled at (currrentActiveOracleprice +-5%) at the settlement block?

Also I didn't find a way to get out of the futures-contract once I opened it. So you can't close your position? which makes sense, since I already know exactly what it will settle on. Like looking 1 week into the future, but not the way that I know where the dToken will stand in 1 week, but by locking the price of the future in 1 week to the currentprice+5%.

@uzyn
Copy link
Contributor

uzyn commented Mar 25, 2022

Thanks @kuegi

I haven't read the code yet. Just responding based on your reviews.

  • Users can create a future contract basically saying I will buy TSLA for X DUSD at the next settlement but without specifying a current price. So when I do this contract, I need to guess what the futures price will be, (up to a week from now). Seems like there is no reason whatsoever to do that more than a few blocks before the settlement (when I know the current price). (In combination with the next point, this kinda "makes sense", but in the way a future should work, this makes no sense)

This is correct. We are not exactly creating a Future here. More like a "future swap".

This serves only to loosely bound dToken to a range. It's also widely discussed that this Future will likely ended up not in use – acting pretty much just like a range guarantee every week.

  • The price where the future settles seems to get set on the previous futures block (to the active oracle price +- rewardPct). So you would say In one week time, the future will settle for the current price +5% right? But this contradicts the whole idea behind the futures. They should settle at the current active oracle price (+- 5%) at the time of settlement, not from the active price 1 week before the settlement. Otherwise this whole thing will lock the price of a dToken within a 5% range of the current price, for the next week.

Shouldn't the future be a pair that can be traded normally and its just fixed that all positions will be settled at (currrentActiveOracleprice +-5%) at the settlement block?

This would be a bug if you're right. I haven't checked the code yet to confirm.

Also I didn't find a way to get out of the futures-contract once I opened it. So you can't close your position? which makes sense, since I already know exactly what it will settle on. Like looking 1 week into the future, but not the way that I know where the dToken will stand in 1 week, but by locking the price of the future in 1 week to the currentprice+5%.

You should be able to get out by getting back your original input before the swap block. I haven't checked the code yet to see if it's implemented, but specification wise you should be able to get back your original input.

@Bushstar
Copy link
Member Author

@kuegi many thanks for your feedback. You are correct, the implementation does need to be updated to use the Oracle price at the point of execution.

@kuegi
Copy link
Contributor

kuegi commented Mar 25, 2022

@kuegi many thanks for your feedback. You are correct, the implementation does need to be updated to use the Oracle price at the point of execution.

Thx for the confirmation. If I understand it correctly, this would mean that we don't need the new "futurePrices" at all, right? (so no datastructure, not storing them, no rpc calls) Cause we only use the active oracle price at the settlement.
Would reduce the size of the change dramatically. And since this is a "safeguard" feature that is probably/hopefully not really used alot, less change is good.

@Bushstar
Copy link
Member Author

@kuegi Correct, but I was thinking of perhaps keeping the listfuturesprices and getfuturesprices and apply the 5% to the current price, this gives people an idea of what to expect, like a helper function.

@kuegi
Copy link
Contributor

kuegi commented Mar 25, 2022

@kuegi Correct, but I was thinking of perhaps keeping the listfuturesprices and getfuturesprices and apply the 5% to the current price, this gives people an idea of what to expect, like a helper function.

Honestly I wouldn't do that, as this opens a wide door for misunderstandings. Users seeing this rpc call, thinking that those prices are fixed already. then they open futures positions based on this assumptions. And have a possibly really bad awakening on the settlement block.
Yes, docu would state it, you can add warnings, but we know how this usually goes. So I wouldn't open that door. Risk/Reward ratio is just not worth it IMHO.

@Bushstar
Copy link
Member Author

@kuegi I'll remove them to save misunderstandings.

if (type == AttributeTypes::Param) {
if (typeId == ParamIDs::DFIP2201) {
if (typeKey == DFIPKeys::RewardPct ||
typeKey == DFIPKeys::BlockPeriod) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since this block in general is prepared for other typeIds etc. Why check here for the "other" values?
This if needs to be adapted whenever a new typeId (with new typeKeys) come along. Would be better to check here for the wanted values and return the error if typeKey is not a wanted one, right?

src/masternodes/govvariables/attributes.cpp Outdated Show resolved Hide resolved
src/masternodes/mn_checks.cpp Outdated Show resolved Hide resolved
src/masternodes/govvariables/attributes.cpp Outdated Show resolved Hide resolved
src/masternodes/mn_checks.cpp Outdated Show resolved Hide resolved
src/masternodes/rpc_accounts.cpp Outdated Show resolved Hide resolved
src/masternodes/mn_rpc.h Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Show resolved Hide resolved
bvbfan
bvbfan previously approved these changes Mar 28, 2022
bvbfan
bvbfan previously approved these changes Mar 28, 2022
@prasannavl prasannavl merged commit 4062fc2 into master Mar 28, 2022
@prasannavl prasannavl deleted the dfip2203-futures branch March 28, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants