Open-Markets-Initiative / wireshark-lua

Source generated cross platform Wireshark dissectors
GNU General Public License v3.0
188 stars 63 forks source link

Minor changes required for the JNX protocols #45

Closed chutzimir closed 1 year ago

chutzimir commented 1 year ago

A few minor issues were spotted on the Jnx.Equities.Pts.Ouch.v1.11 and Jnx.Equities.PTS.Itch.v1.6 protocol dissectors. As suggested, making an issue here with an explanation and justification:

As documented in Section 5 "Data Types" of the OUCH protocol:

  1. Alpha fields are left-justified and padded on the right with spaces.

jnx_equities_pts_itch_v1_6_display.group has a check for if value == "DAY" then but it should be if value == "DAY " then with a trailing space to right pad it to 4 characters.

The same thing exists for jnx_equities_pts_ouch_v1_11_display.group.

E.g.:

--- a/Jnx.Equities.PTS.Itch.v1.6.Script.Dissector.lua
+++ b/Jnx.Equities.PTS.Itch.v1.6.Script.Dissector.lua
@@ -548,7 +548,7 @@ jnx_equities_pts_itch_v1_6_size_of.group = 4

 -- Display: Group
 jnx_equities_pts_itch_v1_6_display.group = function(value)
-  if value == "DAY" then
+  if value == "DAY " then
     return "Group: Daytime Market (DAY)"
   end
   if value == "NGHT" then
  1. Tokens are integers

Integer fields are unsigned big-endian (network byte order) binary encoded numbers. Token fields are 4 byte Integer fields.

Therefore the following fields in jnx_equities_pts_ouch_v1_11 should be UINT32 (they are currently parsed as STRING)

E.g., for order_token the following, replicate for the other tokens:

--- a/Jnx.Equities.Pts.Ouch.v1.11.Script.Dissector.lua
+++ b/Jnx.Equities.Pts.Ouch.v1.11.Script.Dissector.lua
@@ -54,7 +54,7 @@ jnx_equities_pts_ouch_v1_11.fields.order_rejected_message = ProtoField.new("Orde
 jnx_equities_pts_ouch_v1_11.fields.order_rejected_reason = ProtoField.new("Order Rejected Reason", "jnx.equities.pts.ouch.v1.11.orderrejectedreason", ftypes.STRING)
 jnx_equities_pts_ouch_v1_11.fields.order_replaced_message = ProtoField.new("Order Replaced Message", "jnx.equities.pts.ouch.v1.11.orderreplacedmessage", ftypes.STRING)
 jnx_equities_pts_ouch_v1_11.fields.order_state = ProtoField.new("Order State", "jnx.equities.pts.ouch.v1.11.orderstate", ftypes.STRING)
-jnx_equities_pts_ouch_v1_11.fields.order_token = ProtoField.new("Order Token", "jnx.equities.pts.ouch.v1.11.ordertoken", ftypes.STRING)
+jnx_equities_pts_ouch_v1_11.fields.order_token = ProtoField.new("Order Token", "jnx.equities.pts.ouch.v1.11.ordertoken", ftypes.UINT32)
 jnx_equities_pts_ouch_v1_11.fields.orderbook_id = ProtoField.new("Orderbook Id", "jnx.equities.pts.ouch.v1.11.orderbookid", ftypes.UINT32)
 jnx_equities_pts_ouch_v1_11.fields.packet = ProtoField.new("Packet", "jnx.equities.pts.ouch.v1.11.packet", ftypes.STRING)
 jnx_equities_pts_ouch_v1_11.fields.packet_header = ProtoField.new("Packet Header", "jnx.equities.pts.ouch.v1.11.packetheader", ftypes.STRING)
@@ -318,7 +318,7 @@ end
 jnx_equities_pts_ouch_v1_11_dissect.order_token = function(buffer, offset, packet, parent)
   local length = jnx_equities_pts_ouch_v1_11_size_of.order_token
   local range = buffer(offset, length)
-  local value = range:string()
+  local value = range:uint()
   local display = jnx_equities_pts_ouch_v1_11_display.order_token(value, buffer, offset, packet, parent)

   parent:add(jnx_equities_pts_ouch_v1_11.fields.order_token, range, value, display)
  1. Display field is optional, and having a blank value is acceptable.

The accepted values for the Display field of an enter order message is "P" or blank if unused. The dissector shows "unknown value" when blank. The following fix makes it display as intended:

 jnx_equities_pts_ouch_v1_11_display.display = function(value)
   if value == "P" then
     return "Display: Postonly (P)"
   end

+  if value == " " then
+    return "Display: Unused"
+  end
+
   return "Display: Unknown("..value..")"
 end
  1. The filenames style PTS inconsistently. The ITCH one is PTS, while the OUCH one is Pts. PTS would be the preferred styling as it is an abbreviation for Proprietary Trading System.

  2. The link to the specifications is not correct in https://github.com/Open-Markets-Initiative/wireshark-lua/blob/main/Jnx/ReadMe.md. The correct links is https://www.japannext.co.jp/library, but the ReadMe is pointing to https://roadmap.japannext.co.jp/en. image

Open-Markets-Initiative commented 1 year ago

Thanks for all of these. The token issue is known (apparently OUCH types are not universal). We are currently working on Pillar Binary gateways and C-Parsers but we should be able to find a cycles to update the model and regenerate in the near future.

chutzimir commented 1 year ago

Thank you. If it is any help, I pushed manual modifications to the generated Lua dissectors to https://github.com/Japannext/wireshark-lua/commits/jnx-updates

I understand that all code is generated, so take this as a reference of a working version if you'd like.

It is only these commits: at https://github.com/Open-Markets-Initiative/wireshark-lua/compare/main...Japannext:wireshark-lua:jnx-updates?expand=1

chutzimir commented 1 year ago

I confirmed that the changes in https://github.com/Open-Markets-Initiative/wireshark-lua/commit/4fb440ce99a94a7571da3e368d0b80442cff4b42 address some of the issues mentioned here:

chutzimir commented 1 year ago

Your efforts with this project are much appreciated. Feel free to use these sample PCAP files if you ever need to verify the dissectors, or if you want to use them in any automated testing processes.

Soon there will be also samples for the next version of the protocols when the UAT environment is upgraded in August. The change is minor, with the Orderbook Id transitioning from Integer type to Alphanumeric type.

Open-Markets-Initiative commented 1 year ago

Thanks for these pcaps. We will use these for automated testing. We will post the next version of Jnx equities itch and ouch shortly (once we regenerate the spaces issue). And do the bonds protocols time permitting.

Open-Markets-Initiative commented 1 year ago

This should close out this ticket:

https://github.com/Open-Markets-Initiative/wireshark-lua/commit/2ad654cd5a5e2e32669c6f978aafa8a43224a602

I still have to add price precision. Thanks for all your help. Just curious what is your relationship with Jnx? Do you work with other exchanges?

chutzimir commented 1 year ago

This should close out this ticket:

2ad654c

This looks great for the OUCH feed. But the ITCH feeds (for other exchanges, too) need the same treatment. Specifically, when the group field display compares the 4-byte group field value with the 3-byte string "DAY" it would never match.

I still have to add price precision.

Price precision would be nice... divide by 10 and that would be it.

Thanks for all your help. Just curious what is your relationship with Jnx? Do you work with other exchanges?

I am the Chief Information Officer of JNX. Therefore, I must say I truly appreciate what you have done here. While Corvil has good decoders for exchange protocols, every now and then we use Wireshark too :)

Open-Markets-Initiative commented 1 year ago

Turns out systemically handling Ouch precision is not simple since different exchanges have different values. Also, I checked and you are correct that I need to look at the Itch types. Updates shortly.

Open-Markets-Initiative commented 1 year ago

Update Itch spacing and Jnx Ouch price precision: https://github.com/Open-Markets-Initiative/wireshark-lua/commit/5548baa59ecdcf3a7c095575091e9bf943289527