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

js_operation_serializer outdates #1701

Open
xeroc opened this issue Apr 5, 2019 · 18 comments

Comments

@xeroc
Copy link
Member

commented Apr 5, 2019

As a UI developer I want to have https://github.com/bitshares/bitsharesjs/blob/master/lib/serializer/src/operations.js automatically generated by js_operations_serializer from core.

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • Other (please add below)
    JS client development

Cross references:

CORE TEAM TASK LIST

  • Evaluate / Prioritize Bug Report
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation

xeroc added a commit that referenced this issue Apr 5, 2019

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Still missing: #947

@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

There was a (IMHO incomplete) PR #1133. @xeroc perhaps your new code will make it obsolete.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

It was my understanding that the sole purpose of js_operation_serializer is to describe API data structures in a way that can be parsed by bitshares-js.

If the output of js_operation_serializer is different from what bitshares-js uses today, that opens up two questions:

  1. How is the operations.js file in bitshares-js maintained today?
  2. Is js_operation_serializer still required at all? Or put differently: would it be better/easier/desirable for bitshares-js maintenance if the output of js_operation_serializer matched operations.js?
@clockworkgr

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

  1. I believe there is some manual work involved
  2. Probably easier if they did
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@sschiessl-bcp can you comment? Do you need the serializer? Which output format do you need / would you prefer?

@sschiessl-bcp

This comment has been minimized.

Copy link

commented Apr 8, 2019

The serializer comes it handy for adjusting and should be maintained. I have not checked how it deviates, but I assume it is only in the formatting since we have introduced the prettifier hook.

Best case would be if it matches of course, but it would also be accepable to run the prettifier hook manually once before comparing it.

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

This pull request was mostly generated by the serializer in this PR:
https://github.com/bitshares/bitsharesjs/pull/43/files

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Should the serializer produce output identical to that file then?

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Should the serializer produce output identical to that file then?

That would probably be the best, I only made changes so that the output is no longer in CoffeeScript but rather in ES/Javascript. Someone else would need to follow up on this.

You can find a quick diff here:
http://www.mergely.com/ITVZr6ep/

Ignore the stuff at the top. Down the file, lots of whitespace/wrapping differences that could potentially be resovled by jslint

@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I only made changes so that the output is no longer in CoffeeScript but rather in ES/Javascript.

There is a document mentioned a step to convert generated coffee script to javascript: #1701 (comment)

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Edited OP to make this a user story.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

See #1133 for some steps in the right (I hope) direction.

@pmconrad pmconrad added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Apr 12, 2019

@pmconrad pmconrad added this to the Future Feature Release milestone Apr 12, 2019

@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Mentioned in bitsharesjs: https://github.com/bitshares/bitsharesjs/blob/master/lib/serializer/src/operations.js#L59-L61

// ##  Generated code follows
// # programs/js_operation_serializer > npm i -g decaffeinate
// ## -------------------------------

pmconrad added a commit that referenced this issue Apr 29, 2019

@xeroc

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@abitmore thanks for the comment. Closing this pull request.

@xeroc xeroc closed this Apr 30, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

As commented in #1702 I'd like to keep this open for a complete solution.

@dot5enko

This comment has been minimized.

Copy link

commented May 3, 2019

i've been involved into adding htlc features to ui so this also affected adding this ops to bitsharesjs. From what i've seen - everything works perfect, except extensions field of ops. as of now they get converted by default as future_extension type in js, and in most cases - it didn't work. Extensions in c++ have their own type - struct *_extension_type which could be converted into js i guess instead of generic extension "constructor" which you now need to write by your own. I can take over this issue in core project as i wanted to connect to core development process and this issue corefer to my interests now in bitshares-ui.

PS. now output of json_operation_serializer is not usable in bitsharesjs even in any of its PR implementation

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

There are currently no extensions defined for HTLC operations. The extensions field is just a placeholder for future extensions to be added. Insofar the representation as future_extension sounds correct, but I'm not familiar with the js library types.

In any case it would be great if someone from the UI team could take up this issue. You guys know best what you need.

@dot5enko

This comment has been minimized.

Copy link

commented May 14, 2019

@pmconrad im talking not about htlc concretly here. serializer doesn't handle extensions at all. Now output of this program cannot be used directly in bitsharesjs

@abitmore abitmore reopened this May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.