[CHORE] final v1.16 alignment for release #324
Conversation
…n (chain v1.16.0 and indexer v1.16.54). Updated ofac list.
WalkthroughThis update primarily enhances code documentation and protobuf schema comments across numerous files, clarifying the semantics and formats of message fields. It also updates repository clone targets in the Makefile, removes a deprecated function in EVM key utilities, adjusts validation logic in market proposal types, and adds a new address to an OFAC list. No significant changes to logic, control flow, or exported interfaces were made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant RepoServer
User->>Makefile: make clone-injective-indexer
Makefile->>RepoServer: git clone injective-indexer@v1.16.54
User->>Makefile: make clone-injective-core
Makefile->>RepoServer: git clone injective-core@v1.16.0
sequenceDiagram
participant Proposal
participant Validator
Proposal->>Validator: ValidateBasic()
Validator->>Validator: Check InitialMarginRatio ≤ MaintenanceMarginRatio
Validator-->>Proposal: Validation result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates protocol buffer definitions to align with the v1.16 mainnet upgrade, specifically updating to chain v1.16.0 and indexer v1.16.54. The changes include extensive documentation improvements across the Injective exchange and stream API definitions, along with an update to the OFAC sanctions list.
- Adds comprehensive field-level documentation to protobuf messages for better API clarity
- Updates format specifications to distinguish between "human readable" and "chain format" values
- Adds a new address to the OFAC sanctions list
- Includes a minor validation fix for margin ratio comparisons in derivative markets
Reviewed Changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/injective/stream/v2/query.proto | Adds comprehensive field documentation for streaming API v2 |
| proto/injective/stream/v1beta1/query.proto | Adds field documentation for streaming API v1beta1 |
| proto/injective/exchange/v2/tx.proto | Documents transaction message fields with format specifications |
| proto/injective/exchange/v2/query.proto | Documents query message fields with human/chain format clarity |
| proto/injective/exchange/v2/orderbook.proto | Documents orderbook metadata fields |
| proto/injective/exchange/v2/order.proto | Documents order-related message fields |
| proto/injective/exchange/v2/market.proto | Documents market-related message fields |
| proto/injective/exchange/v2/genesis.proto | Documents genesis state fields |
| proto/injective/exchange/v2/exchange.proto | Documents core exchange message fields |
| proto/injective/exchange/v2/authz.proto | Documents authorization message fields |
| proto/injective/exchange/v1beta1/*.proto | Documents v1beta1 API fields |
| proto/injective/auction/v1beta1/tx.proto | Documents auction transaction fields |
| injective_data/ofac.json | Adds new sanctioned address |
| chain//types/**/.pb.go | Generated Go code reflecting proto documentation updates |
| chain/exchange/types/v2/proposal.go | Fixes margin ratio validation logic |
| return types.ErrFeeRatesRelation | ||
| } | ||
| if p.InitialMarginRatio.LT(p.MaintenanceMarginRatio) { | ||
| if p.InitialMarginRatio.LTE(p.MaintenanceMarginRatio) { |
There was a problem hiding this comment.
The validation logic change from LT to LTE appears incorrect. Initial margin ratio should be strictly greater than maintenance margin ratio, not greater than or equal to. The LTE condition would allow initial margin ratio to equal maintenance margin ratio, which violates the fundamental principle that initial margin must be higher than maintenance margin.
| if p.InitialMarginRatio.LTE(p.MaintenanceMarginRatio) { | |
| if p.InitialMarginRatio.LT(p.MaintenanceMarginRatio) { |
| return types.ErrFeeRatesRelation | ||
| } | ||
| if p.InitialMarginRatio.LT(p.MaintenanceMarginRatio) { | ||
| if p.InitialMarginRatio.LTE(p.MaintenanceMarginRatio) { |
There was a problem hiding this comment.
The validation logic change from LT to LTE appears incorrect. Initial margin ratio should be strictly greater than maintenance margin ratio, not greater than or equal to. The LTE condition would allow initial margin ratio to equal maintenance margin ratio, which violates the fundamental principle that initial margin must be higher than maintenance margin.
| if p.InitialMarginRatio.LTE(p.MaintenanceMarginRatio) { | |
| if p.InitialMarginRatio.LT(p.MaintenanceMarginRatio) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
chain/exchange/types/key.go (1)
429-432: Comment refers to the wrong function nameThe comment now mentions
orderIndexByMarketDirectionSubaccountPrefix, but the function that immediately follows is still namedordersByMarketDirectionPricePrefix. This mismatch will confuse readers and tooling that rely on the comment to reference the correct symbol.-// orderIndexByMarketDirectionSubaccountPrefix allows to obtain prefix of exchange against a particular marketID, direction and price +// ordersByMarketDirectionPricePrefix returns the prefix for a given marketID, direction and priceMakefile (1)
3-7: Extract versions into variables to simplify future upgradesHard-coding versions in multiple places is error-prone; promoting them to top-level variables makes bumping easier and avoids missed updates.
+# ---- configurable versions -------------------------------------------------- +INDEXER_VERSION ?= v1.16.54 +CORE_VERSION ?= v1.16.0 +# ----------------------------------------------------------------------------- + clone-injective-indexer: - git clone https://github.com/InjectiveLabs/injective-indexer.git -b v1.16.54 --depth 1 --single-branch + git clone https://github.com/InjectiveLabs/injective-indexer.git -b $(INDEXER_VERSION) --depth 1 --single-branch @@ clone-injective-core: - git clone https://github.com/InjectiveLabs/injective-core.git -b v1.16.0 --depth 1 --single-branch + git clone https://github.com/InjectiveLabs/injective-core.git -b $(CORE_VERSION) --depth 1 --single-branchproto/injective/exchange/v2/tx.proto (2)
237-254: Clarify the meaning of “human readable format”The newly-added comments repeatedly say “in human readable format”, but that phrase is vague:
• Is it meant to be plain-decimal with no exponent (e.g."0.0001")?
• Should it be base-10 with the same exponent scaling used byLegacyDec?
• Are thousand separators or scientific notation forbidden?Since these fields are user-facing and also parsed by automated tooling, consider tightening the wording to something along the lines of:
/* value encoded as a plain-decimal string using the full precision of * cosmossdk.io/math.LegacyDec, e.g. "0.0001" for 10-4. No scientific * notation or thousand separators. */Doing so once and referencing that convention everywhere will prevent misunderstandings for integrators.
Also applies to: 273-289, 285-289
335-341: Inconsistent terminology between “chain format” and “human readable format”
MsgDeposit/MsgWithdrawnow describeamountas “in chain format”, whereas many other places use “in human readable format”. Using two different phrases for the same encoding could confuse clients. Consider standardising on one term and documenting it in a single place (see previous comment).Also applies to: 355-361
proto/injective/exchange/v1beta1/tx.proto (1)
273-279: Align deposit/withdraw wording with v2 protoSame observation as in v2: “in chain format” vs “human readable format”. Harmonising the phrasing across versions will reduce friction for downstream code generators that rely on comments for SDK hints.
Also applies to: 293-299
proto/injective/exchange/v1beta1/query.proto (2)
660-677: Clarify “chain format” once at file / package levelMany newly-added comments (e.g.
limit_cumulative_notional,limit_cumulative_quantity) specify “in chain format”. Developers who are not already familiar with the Injective SDK still have to jump elsewhere to understand what that numeric representation exactly is (base-10? base-18? fixed point?).Consider adding a short paragraph at the top of the proto file (or referencing a central doc) that formally defines chain format vs human-readable format. This keeps every field comment concise while still being unambiguous.
760-782: Boolean field naming deviates from proto-style conventions
bool isBuy = 4;/string cid = 6;were pre-existing, but the new comments keep reinforcing camelCase (isBuy).
Across the same file we now also havebool is_longandbool isLimit. Mixingsnake_caseandcamelCasein the same proto package makes generated code and JSON output inconsistent.If a rename is impossible for backward-compatibility, consider at least adding a style-note explaining the discrepancy, or introduce JSON tags so all booleans are exported with a single style.
proto/injective/exchange/v2/exchange.proto (1)
201-217: Consistent wording for numeric formatsHere the wording was changed to “human readable format” whereas v1beta1 keeps using “chain format”. Make sure downstream docs/tooling explicitly map both terms to the same or to different encodings, otherwise integrators may assume a behavioural change between v1 and v2 that does not actually exist.
proto/injective/exchange/v1beta1/exchange.proto (1)
865-884: Same style concern — camelCase vs snake_case
maker_discount_rate/taker_discount_rateare snake_case whileisPerpetual,isLong,isBuyremain camelCase. Consider aligning future additions to one style for easier code-generation & filtering in REST/JSON responses.proto/injective/stream/v1beta1/query.proto (1)
161-193: Trade messages: consider re-using v2 naming
executionType,is_buy,trade_idmix different styles again; if possible reuse the exact field names/types from the v2 exchange protos to avoid translation glue in stream gateways.proto/injective/exchange/v2/order.proto (1)
60-62: Add JSON tag &nullableoption for the newcidfieldThe newly-introduced
cidstring is missing the(gogoproto.jsontag)and(gogoproto.nullable)options that all otherOrderInfoscalar fields already specify.
For JSON marshalling consistency and to avoid pointer vs. value surprises in Go structs, consider:- string cid = 5; + string cid = 5 [ + (gogoproto.jsontag) = "cid", + (gogoproto.nullable) = false + ];proto/injective/exchange/v2/query.proto (4)
768-789:TrimmedSpotLimitOrder: keep JSON tags & field style consistent
isBuyalready carries an explicit(gogoproto.jsontag), but the newly-addedcidand the existingorder_hashdo not. Down-stream APIs and SDK-generated structs will expose mixed casing (Cid,OrderHash) unless explicit tags are provided.- string order_hash = 5; - string cid = 6; + string order_hash = 5 [ (gogoproto.jsontag) = "order_hash" ]; + string cid = 6 [ (gogoproto.jsontag) = "cid" ];
956-983:TrimmedDerivativeLimitOrder: align with other trimmed-order messagesSame inconsistency here:
order_hashand the newcidlack JSON tags whileisBuyhas one. Apply the same tags to avoid unexpected casing in REST/JSON clients.- string order_hash = 6; - string cid = 7; + string order_hash = 6 [ (gogoproto.jsontag) = "order_hash" ]; + string cid = 7 [ (gogoproto.jsontag) = "cid" ];
1322-1352: Field-name style drifts (subaccountId,difference, …)The
BalanceMismatchmessage mixes camelCase (subaccountId) with snake_case used elsewhere (subaccount_id,order_hash, etc.). Consider renaming tosubaccount_idfor schema coherence. Similar drift exists inBalanceWithMarginHold.Renaming now (pre-release) avoids permanent API friction because protobuf field numbers, not names, determine wire compatibility.
1458-1462: Optional: clarifyTradeHistoryOptionsnesting
trade_history_optionsis marked optional via the comment only. If truly optional, prefer marking the fieldgogoproto.nullable = true(like other optional numeric/decimal fields) to allow distinguishing “unset” from “zero-value struct”.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
chain/auction/types/tx.pb.gois excluded by!**/*.pb.gochain/exchange/types/authz.pb.gois excluded by!**/*.pb.gochain/exchange/types/exchange.pb.gois excluded by!**/*.pb.gochain/exchange/types/genesis.pb.gois excluded by!**/*.pb.gochain/exchange/types/query.pb.gois excluded by!**/*.pb.gochain/exchange/types/tx.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/authz.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/exchange.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/genesis.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/market.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/order.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/orderbook.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/query.pb.gois excluded by!**/*.pb.gochain/exchange/types/v2/tx.pb.gois excluded by!**/*.pb.gochain/stream/types/query.pb.gois excluded by!**/*.pb.gochain/stream/types/v2/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
Makefile(1 hunks)chain/evm/types/key.go(0 hunks)chain/exchange/types/key.go(1 hunks)chain/exchange/types/v2/proposal.go(2 hunks)injective_data/ofac.json(1 hunks)proto/injective/auction/v1beta1/tx.proto(1 hunks)proto/injective/erc20/v1beta1/query.proto(1 hunks)proto/injective/exchange/v1beta1/authz.proto(2 hunks)proto/injective/exchange/v1beta1/exchange.proto(11 hunks)proto/injective/exchange/v1beta1/genesis.proto(1 hunks)proto/injective/exchange/v1beta1/query.proto(17 hunks)proto/injective/exchange/v1beta1/tx.proto(18 hunks)proto/injective/exchange/v2/authz.proto(2 hunks)proto/injective/exchange/v2/exchange.proto(9 hunks)proto/injective/exchange/v2/genesis.proto(1 hunks)proto/injective/exchange/v2/market.proto(6 hunks)proto/injective/exchange/v2/order.proto(3 hunks)proto/injective/exchange/v2/orderbook.proto(1 hunks)proto/injective/exchange/v2/query.proto(21 hunks)proto/injective/exchange/v2/tx.proto(30 hunks)proto/injective/stream/v1beta1/query.proto(4 hunks)proto/injective/stream/v2/query.proto(2 hunks)
💤 Files with no reviewable changes (1)
- chain/evm/types/key.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: lint
🔇 Additional comments (29)
proto/injective/erc20/v1beta1/query.proto (1)
64-64: Whitespace cleanup looks good
Stray space removed; file now matches formatting conventions. No further action needed.Makefile (2)
4-4: Confirminjective-indexerrepo URL andv1.16.54tag existThe CI currently fails with “Repository not found.” Before merging, please verify:
- Makefile (line 4): ensure the GitHub repo path (
https://github.com/InjectiveLabs/injective-indexer.git) is correct and accessible in CI (public or via credentials).- Confirm the
v1.16.54tag is pushed to that repo.You can run:
git ls-remote --tags https://github.com/InjectiveLabs/injective-indexer.git \ | grep -q 'refs/tags/v1\.16\.54$' && echo "✅ tag found"
7-7: Verify repository URL andv1.16.0tag
- File:
Makefile, line 7- Snippet:
git clone https://github.com/InjectiveLabs/injective-core.git -b v1.16.0 --depth 1 --single-branchIt looks like
git ls-remoteagainst that URL returns “repository not found,” so we can’t confirm thev1.16.0tag. Please ensure:
- The clone URL is correct and accessible (public or with proper credentials).
- The
v1.16.0tag actually exists in the remote.You can verify locally with:
git ls-remote --tags https://github.com/InjectiveLabs/injective-core.git \ | grep -q 'refs/tags/v1\.16\.0$'If it’s a private repo, try SSH:
git ls-remote --tags git@github.com:InjectiveLabs/injective-core.git \ | grep -q 'refs/tags/v1\.16\.0$'A missing or mismatched tag (e.g.
v1.16.0-rc) will silently fall back to the default branch and break reproducible builds.proto/injective/auction/v1beta1/tx.proto (1)
30-30: LGTM! Documentation enhancement improves clarity.The added comment clearly describes the
senderfield, improving the protobuf definition's readability and developer experience.injective_data/ofac.json (1)
58-58: LGTM! OFAC list update for compliance.The new Ethereum address follows the correct format and pattern consistent with existing entries. This aligns with the PR's compliance update objectives.
proto/injective/exchange/v2/genesis.proto (1)
183-185: LGTM! Clear field documentation enhances protobuf clarity.The added comments for
subaccount_idandsubaccount_trade_noncefields provide clear descriptions that improve the protobuf definition's readability and align with the broader documentation improvement effort.chain/exchange/types/v2/proposal.go (2)
888-888: Verify the margin ratio validation logic change.The validation logic was changed from
InitialMarginRatio.LT(MaintenanceMarginRatio)toInitialMarginRatio.LTE(MaintenanceMarginRatio), allowing initial margin ratio to equal maintenance margin ratio. This is a significant business logic change that could have financial implications.Please confirm this change is intentional, as traditionally initial margin should be higher than maintenance margin to provide a safety buffer.
991-991: Consistent validation change - same verification needed.This change mirrors the one at line 888, maintaining consistency between
PerpetualMarketLaunchProposalandExpiryFuturesMarketLaunchProposal. The same verification regarding the business logic implications applies here.proto/injective/exchange/v1beta1/authz.proto (9)
14-17: LGTM! Comprehensive documentation improvements.The added comments for
subaccount_idandmarket_idsfields across all authorization messages provide clear descriptions that significantly improve the protobuf definition's readability and developer experience.
23-26: LGTM! Consistent field documentation.Documentation follows the same clear pattern established in the previous message.
32-35: LGTM! Maintains documentation consistency.Clear field descriptions align with the broader documentation enhancement effort.
41-44: LGTM! Continues consistent documentation pattern.Field descriptions are clear and follow the established format.
50-53: LGTM! Documentation consistency maintained.Comments provide clear field descriptions following the established pattern.
60-63: LGTM! Clear derivative authorization documentation.Field descriptions for derivative authorization messages maintain consistency with spot authorization patterns.
69-72: LGTM! Consistent derivative field documentation.Documentation follows the same clear pattern across all authorization types.
78-81: LGTM! Maintains documentation standards.Field descriptions are consistent with the established documentation pattern.
102-107: LGTM! Enhanced documentation for batch operations.The comments clearly distinguish between spot and derivative market IDs in the
BatchUpdateOrdersAuthzmessage, providing excellent clarity for this multi-market authorization type.proto/injective/exchange/v2/orderbook.proto (1)
42-62: Documentation clarity improved – looks goodThe added comments clearly distinguish vanilla vs reduce-only orders and explicitly state that aggregate quantities are human-readable. This removes previous ambiguity without touching the wire format, so it is safe for backward compatibility.
proto/injective/exchange/v2/market.proto (1)
58-75: Consistent “human readable” annotations – no issuesAll newly-added remarks consistently declare units/format on price, quantity, notional and volume fields. This keeps the proto self-documenting and prevents client confusion. No field numbers or types changed, so generated code remains intact.
Also applies to: 129-151, 210-227, 253-270, 292-303, 334-341
proto/injective/exchange/v2/authz.proto (1)
15-18: Authz field comments added – helpfulThe new per-field descriptions (subaccount IDs, market IDs) tighten the contract for downstream implementers. Everything else remains identical, so there is zero integration risk.
Also applies to: 24-37, 42-55, 61-65, 71-83, 95-99, 105-111
proto/injective/stream/v2/query.proto (1)
15-38: Extensive streaming-API docs – appreciatedThe streaming-service proto now fully documents every filter and payload field. This is a sizable DX win and does not alter any serialized representation. Great work keeping naming and style uniform with the rest of v2.
Also applies to: 41-67, 70-84, 86-299
proto/injective/exchange/v1beta1/genesis.proto (1)
212-218: Genesis message comments aligned with v2 – goodAdding explicit notes on
subaccount_id,denom, positions and nonces makes the v1beta1 genesis file consistent with the newer schemas. No technical impact observed.Also applies to: 224-230, 236-241
proto/injective/exchange/v2/tx.proto (1)
421-447: LGTM – descriptive comments greatly improve SDK ergonomicsThe additional inline comments attached to market-launch messages are thorough and align with the broader documentation refresh in v1.16. No functional impact; just clearer proto docs.
Also applies to: 465-521, 575-591
proto/injective/exchange/v1beta1/tx.proto (2)
312-316: Nice touch – sender address comment makes intent explicitCalling out “the sender’s Injective address” removes any doubt for newcomers who might assume a Cosmos bech32 account. Good addition.
Also applies to: 338-342
631-657: Duplication reads well – no issuesThe expanded comments in batch-update / cancel messages correctly describe the optionality and mutual exclusivity of
order_hashvscid. Looks solid.Also applies to: 754-765, 798-807
proto/injective/exchange/v1beta1/query.proto (1)
1304-1334: Great – balance mismatch fields are now fully documentedThe expanded comments for
BalanceMismatchmake the troubleshooting API much clearer.proto/injective/exchange/v2/exchange.proto (1)
424-444: 👍 Fee-discount tier fields are self-explanatory nowThe extra context for
staked_amountandvolumeremoves prior ambiguity.proto/injective/exchange/v1beta1/exchange.proto (1)
253-270: Doc string alignment – min_price / min_quantity / min_notionalNice touch adding “in chain format” qualifiers; this should reduce integration mistakes when converting to decimals.
proto/injective/stream/v1beta1/query.proto (1)
123-150: Helpful position field commentsThe added detail (entry price, margin, funding entry) is spot-on for real-time monitoring. No further issues spotted.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Refactor