Add cloid to order and cancel requests

Nayshins commented 8 months ago

Tested using the updated order and cancel script. I think the cloid was causing signing issues with the request in #6, and this should allow us to add the modify requests.

lmlmt commented 7 months ago

FYI new hashing is merged

Nayshins commented 7 months ago

Whew... the original hashing scheme definitely presented some challenges to integration, but the new way forward seems much better. I can rebase off of the new changes, and get this working.

Nayshins commented 7 months ago

@lmlmt Updated to use the new signing. Massively cleaner impl

dennohpeter commented 7 months ago

Some suggestions on cloid, cloid can be of type H128 from ethers, removing the need for an extra uuid dependency. To generate a new cloud is pretty simple: H128::random()

Nayshins commented 7 months ago

Bench Results:

uuid v4                 time:   [512.99 ns 533.09 ns 553.06 ns]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

H128::random            time:   [4.9576 µs 5.0533 µs 5.1658 µs]
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) high mild
  6 (6.00%) high severe

lmlmt commented 7 months ago

Some suggestions on cloid, cloid can be of type H128 from ethers, removing the need for an extra uuid dependency. To generate a new cloud is pretty simple: H128::random()

Yeah good idea, and also based on the bench results; feel free to push latest changes here and will take a look

Nayshins commented 7 months ago

Some suggestions on cloid, cloid can be of type H128 from ethers, removing the need for an extra uuid dependency. To generate a new cloud is pretty simple: H128::random()

Yeah good idea, and also based on the bench results; feel free to push latest changes here and will take a look

Bench shows that UUID is 10x faster than H128 500ns vs 5 micros. I think what I have now works the best IMO.

lmlmt commented 7 months ago

Some suggestions on cloid, cloid can be of type H128 from ethers, removing the need for an extra uuid dependency. To generate a new cloud is pretty simple: H128::random()

Yeah good idea, and also based on the bench results; feel free to push latest changes here and will take a look

Bench shows that UUID is 10x faster than H128 500ns vs 5 micros. I think what I have now works the best IMO.

Ah, misread, will review

lmlmt commented 7 months ago

Also confirming you ran the other examples locally and ensured they worked, ie no signature errors?

Nayshins commented 7 months ago

Ran order_and_cancel, order_and_cancel_cloid, and market_maker without issue

Nayshins commented 7 months ago

@lmlmt All cleaned up with tests added. Merge at your convenience!

dennohpeter commented 7 months ago

Bench Results:

uuid v4                 time:   [512.99 ns 533.09 ns 553.06 ns]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

H128::random            time:   [4.9576 µs 5.0533 µs 5.1658 µs]
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) high mild
  6 (6.00%) high severe

Yeah uuid v4 is definitely a lot faster

dennohpeter commented 7 months ago

I have also opened a PR that improves your PR's internal performance when converting uuid to hex. All the tests and function signatures remain the same but with ~2.3x speed boost. Have a look #14

Nayshins commented 7 months ago

Smart! I just left it as the original implementation I had to deal with using the original hashing scheme.

dennohpeter commented 7 months ago

oh! gotcha 👍🏾

hyperliquid-dex / hyperliquid-rust-sdk

Add cloid to order and cancel requests #10