Skip to content
This repository was archived by the owner on Mar 24, 2025. It is now read-only.

feat: add metadata field to Ticker and MarketConfig #128

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

aljo242
Copy link
Contributor

@aljo242 aljo242 commented Feb 14, 2024

Closes BLO-913

Adds metadata fields to msg index 15 for both messages. This gives room for the messages to be extended if necessary, while using a field index that still takes one byte to encode.

Add IsValid() function to new pkg/json package. This checks the basic validity of if a byte array is json.

The metadata fields are chosen as string and not bytes in the proto, because strings can be compared as map keys.

fix
@aljo242
Copy link
Contributor Author

aljo242 commented Feb 14, 2024

One other thing to note is that we probably should choose some length limit for the JSON strings. I'm not sure what that limit would look like and would like to discuss this.

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a904d5a) 74.96% compared to head (b3ee976) 74.99%.

Additional details and impacted files
@@                Coverage Diff                 @@
##           feat/marketmap     #128      +/-   ##
==================================================
+ Coverage           74.96%   74.99%   +0.02%     
==================================================
  Files                 147      148       +1     
  Lines                5959     5966       +7     
==================================================
+ Hits                 4467     4474       +7     
  Misses               1123     1123              
  Partials              369      369              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Eric-Warehime Eric-Warehime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In terms of adding limits to the json size...since it's a string I guess it's slightly harder to reason about encoding size? Maybe just saying 1K and seeing if we ever need to break that is reasonable. Since the updates are currently constrained by the permissioned actor and in the future might have some permissionless element to them, I don't think not validating this poses and huge risk.

@aljo242 aljo242 merged commit 7fd231d into feat/marketmap Feb 15, 2024
6 checks passed
@aljo242 aljo242 deleted the refactor/meta branch February 15, 2024 23:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants