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

Change call_order_update_operation to return order_id #1269

Closed
abitmore opened this Issue Aug 21, 2018 · 8 comments

Comments

Projects
5 participants
@abitmore
Member

abitmore commented Aug 21, 2018

User Story

when there is a margin call fill order one of the fill orders references a 1.8.X call order which I can't find ever being created previously. This is messing with what I'm working at the moment which tracks orders and the time lines they're open for etc.

ID of call orders aren't returned when they're created nor updated:

void_result call_order_update_evaluator::do_apply(const call_order_update_operation& o)

It will help if the IDs are returned.

Note:

  • operation_result is stored in block database, that means they won't be updated after a block get written to disk. If this feature is implemented, need a resync to update old data. Related code:
    struct signed_block : public signed_block_header
    {
    checksum_type calculate_merkle_root()const;
    vector<processed_transaction> transactions;
    };
  • operation_result is being transmitted in P2P network since it's part of block_message, good news is they're not being verified so are not a part of consensus. In this code a processed_transaction is being used as a signed_transaction thus the operation_results field will be ignored:
    processed_transaction database::apply_transaction(const signed_transaction& trx, uint32_t skip)

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
    • Assigned: @cogutvalera
    • Estimated: 2 hours
    • Remitted: Week 39
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@xeroc

This comment has been minimized.

Member

xeroc commented Aug 21, 2018

Why would we not extend the operation and let the user provide a 32bit value indexed in the database so orders can be identified by user-provided value?
Pretty much like it is done in Steem.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 21, 2018

@xeroc that's a good idea, but it needs a BSIP (#556). By the way, our object subscription/notification subsystem doesn't work well with self defined IDs so far. In the mean while, it's not hard for users to attach a custom_operation with arbitrary data to each operation, in the same transaction, for labeling purpose.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Sep 10, 2018

@ryanRfox I want to claim this issue.

Thanks !

@ryanRfox

This comment has been minimized.

Member

ryanRfox commented Sep 10, 2018

@cogutvalera I've assigned this to you. Please provide an estimate prior to development. We want @bitshares/core-dev to review these. Please being with some test cases to ensure you are heading toward the intended solution. Thanks

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Sep 11, 2018

@ryanRfox my estimation for this issue is approximately 2 hours

cogutvalera added a commit to cogutvalera/bitshares-core that referenced this issue Sep 30, 2018

pmconrad added a commit that referenced this issue Oct 1, 2018

Merge pull request #1352 from cogutvalera/issue_1269
Change call_order_update_operation to return order_id #1269
@pmconrad

This comment has been minimized.

pmconrad commented Oct 1, 2018

Resolved through #1352

@pmconrad pmconrad closed this Oct 1, 2018

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Oct 1, 2018

Thanks !

@abitmore abitmore removed this from New -Awaiting Core Team Evaluation in Project Backlog Oct 1, 2018

@abitmore abitmore added this to To do in Feature release (201810) via automation Oct 1, 2018

@abitmore abitmore moved this from To do to Done in Feature release (201810) Oct 1, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Nov 20, 2018

Update (important info):

  • when a node received a block, it saves the block to disk "as is" after verified, so a resync won't cause a change in block database file;
  • operation_result is used when calculating merkle tree, since merkle tree root is used to calculate block id, operation results of transactions in existing blocks should not be changed, otherwise old blocks will become invalid thus nodes will be unable to sync;
  • operation results offered by witnesses are not verified; that said, operation_result stored in 1.11.x objects and those stored in blocks can be different e.g. due to different node versions (due to PR for this issue); we can add a check for this with a hard fork.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment