-
Notifications
You must be signed in to change notification settings - Fork 649
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
BSIP38 add target_cr option to call order #838
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
82c8d31
BSIP38: add target_CR option to call order #834
abitmore 51a2aff
BSIP38: update limit:call matching logic #834
abitmore 7c84535
BSIP38: update call:limit matching logic #834
abitmore a60573e
BSIP38: add CLI command borrow_asset_ext #834
abitmore 96b85ad
BSIP38: update rounding when calculating maximums
abitmore ec00b2a
BSIP38: adjust maximums calculation, add comments
abitmore 7d5db49
BSIP38 tests: add target_CR to database fixture
abitmore 49e5a59
BSIP38 tests: object, validation, hardfork time
abitmore 087e564
BSIP38 tests: add random tests
abitmore 2c61ad9
BSIP38: handle rounding when calculating maximums
abitmore b4b18e2
Add random tests for asset multiplies price
abitmore ddb6baa
BSIP38 tests: update to hit more edge cases
abitmore dd8d7f6
BSIP38 tests: move test case to new file, refactor
abitmore 15b2b16
BSIP38: update algorithm when calculating maximums
abitmore 58542b8
Merge bsip35 changes into bsip38 branch
abitmore 464fd38
Use asset::multiply_and_round_up() in BSIP38
abitmore e68df58
BSIP38 tests: fewer iterations in call_order_tests
abitmore a1e9d89
Update comments and dup vars according to review
abitmore e7f4af8
Merge bsip35 changes into bsip38 branch
abitmore baaf170
BSIP38: update extensions type; add proposal test
abitmore d1c3448
BSIP38: get_max... in call_obj returns debt only
abitmore e2f33f5
Merge hardfork branch into bsip38 branch
abitmore 516228b
BSIP38 tests: proposal test after hard fork
abitmore 6a43799
BSIP38 tests: add extreme case: mcr 1002 mssr 1001
abitmore 182e6ec
BSIP38 tests: add order matching and filling test
abitmore 610dbf5
Fix BSIP38 test case about hard fork time check
abitmore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
// bitshares-core issue #834 "BSIP38: add target CR option to short positions" | ||
#ifndef HARDFORK_CORE_834_TIME | ||
#define HARDFORK_CORE_834_TIME (fc::time_point_sec( 1600000000 )) | ||
#endif |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The usual semantics of *_update operations is "if absent leave untouched", while here it is "if absent delete". This is confusing.
Perhaps make the extension field an optional<optional<uint16_t>>, or use a special value like 0 for deletion.
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 don't think
optional<optional<uint16_t>>
can be jsonified correctly.Since type of that field is
optional
already, it would be always confusing about how to delete it, IMHO it's easier to treat it as an always-required field, aka always update.Another approach is to use 2 fields, one for existence and the other for the real value. But I think it's too complicated since the
optional
type is just for doing this.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'd still prefer to keep the "if absent leave untouched" semantic. I suppose the option will not be available in the UI (all UI's) very soon, so someone who is used to trade via UI might want to set the tcr once via cli_wallet and continue normal operation using the UI.
Since tcr < 1000 doesn't make sense anyway, you could treat and invalid value (i. e. < 1000) as a deliberate delete request.
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 still think it's clearer to just update as is (which is written in BSIP).
Compare the two approaches:
IMHO the 2nd one will lead to more confusion.
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.
Another thing related is that
call_order_update_operation
is not only for updating a call order, but also for creating and removing, although removing is irrelevant here.If the option is in "if absent leave untouched" semantic, the user may expect that the system will "remember" the option when a position is closed (either intendedly or unintendedly), also sometimes may forget to set when recreating a new position, both may lead to unintended behavior.
Compare the two approaches (I think the 1st one is better):
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.
Well, seems we have to agree to disagree (again ;-) ).
BSIP is authoritative though, so I'm OK with leaving it as it is.
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.
Perhaps propose a BSIP change? E.G. drop
optional
from object, use value0
as "not set" instead.