Conversation
📝 WalkthroughWalkthroughBumps package/submodule; adds a generic Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/injective-std/src/types/cometbft/rpc/grpc/v1beta1.rs (1)
18-25:⚠️ Potential issue | 🟠 Major
ResponseBroadcastTxis missingHashunlike its sibling types.This struct doesn't derive
HashwhileRequestPing,RequestBroadcastTx, andResponsePingdo. The nested field types (ResponseCheckTx,ResponseDeliverTx) also don't implementHash, preventingResponseBroadcastTxfrom deriving it. To addHashhere, the nested types must derive it first.
🧹 Nitpick comments (5)
packages/injective-std/src/types/injective/auction/v1beta1.rs (1)
84-105: Consider derivingHashfor the remaining message types in this module.
EventAuctionStartandQueryCurrentAuctionBasketResponsestill lackHashwhile adjacent message types now include it. If the intent is module-wide hashability, aligning these two improves API consistency.Suggested patch
-#[derive(Clone, PartialEq, Eq, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] pub struct EventAuctionStart {-#[derive(Clone, PartialEq, Eq, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] pub struct QueryCurrentAuctionBasketResponse {Also applies to: 162-188
packages/proto-build-injective/src/transformers.rs (1)
772-794: Add tests for enum renames + enum deserializer injection.The new enum rename logic and
allow_serde_enum_str_or_inthelper aren’t covered by tests yet. Please add targeted tests to lock in serde rename/alias emission and the enum deserializer attribute insertion to avoid regressions.packages/injective-std/src/types/injective/peggy/v1.rs (1)
765-795: Consider deriving Hash for RateLimit to match other public types.
Most public Peggy structs now derive Hash; adding it here avoids trait-bound surprises when RateLimit is used in HashMap/HashSet.♻️ Suggested change
-#[derive(Clone, PartialEq, Eq, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] pub struct RateLimit {packages/injective-std/src/types/cosmos/auth/module/v1.rs (1)
3-15: Consider addingHashtoModulefor consistency.Now that
ModuleAccountPermissionderivesHash, theModulestruct could also deriveHashsince all its fields would be hashable (String,Vec<ModuleAccountPermission>,String). This would maintain consistency with the PR's goal of expandingHashderives across types.♻️ Optional: Add Hash to Module
-#[derive(Clone, PartialEq, Eq, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)] +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message, ::serde::Serialize, ::serde::Deserialize, ::schemars::JsonSchema, CosmwasmExt)]packages/injective-std/src/types/cometbft/abci/v1beta1.rs (1)
576-594: Missing serde attributes on someresponse_apply_snapshot_chunk::Resultvariants.The variants
Retry,RetrySnapshot, andRejectSnapshot(lines 589-593) lack the#[serde(rename)]and#[serde(alias)]attributes that other variants in this enum have. This creates an inconsistency whereUnknown,Accept, andAbortserialize/deserialize with uppercase names and PascalCase aliases, while the remaining variants use default serde behavior.If this is intentional for backward compatibility, please disregard. Otherwise, consider adding the missing attributes for consistency:
♻️ Suggested fix for consistency
/// Retry chunk (combine with refetch and reject) + #[serde(rename = "RETRY")] + #[serde(alias = "Retry")] Retry = 3, /// Retry snapshot (combine with refetch and reject) + #[serde(rename = "RETRY_SNAPSHOT")] + #[serde(alias = "RetrySnapshot")] RetrySnapshot = 4, /// Reject this snapshot, try others + #[serde(rename = "REJECT_SNAPSHOT")] + #[serde(alias = "RejectSnapshot")] RejectSnapshot = 5,
| #[prost(enumeration = "ClaimType", tag = "1")] | ||
| #[serde( | ||
| serialize_with = "crate::serde::as_str::serialize", | ||
| deserialize_with = "crate::serde::as_str::deserialize" | ||
| )] | ||
| #[serde(deserialize_with = "crate::serde::enum_i32::deserialize::<ClaimType, _>")] | ||
| pub attestation_type: i32, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "enum_i32::(de|ser)erialize" -g '*.rs'Repository: InjectiveLabs/injective-rust
Length of output: 54
🏁 Script executed:
rg -n "EventAttestationObserved|EventValidatorJailed" -g '*.rs'Repository: InjectiveLabs/injective-rust
Length of output: 527
🏁 Script executed:
wc -l packages/injective-std/src/types/injective/peggy/v1.rsRepository: InjectiveLabs/injective-rust
Length of output: 131
🏁 Script executed:
sed -n '251,260p' packages/injective-std/src/types/injective/peggy/v1.rsRepository: InjectiveLabs/injective-rust
Length of output: 527
🏁 Script executed:
sed -n '629,640p' packages/injective-std/src/types/injective/peggy/v1.rsRepository: InjectiveLabs/injective-rust
Length of output: 564
🏁 Script executed:
find . -type f -name "*.rs" -path "*/serde/*" | head -20Repository: InjectiveLabs/injective-rust
Length of output: 113
🏁 Script executed:
cat packages/injective-std/src/serde/mod.rsRepository: InjectiveLabs/injective-rust
Length of output: 2687
Add serialize helper for enum_i32 fields to match deserialization contract.
The enum_i32 serde helper only implements deserialize, not serialize. This causes JSON output to emit numeric i32 values instead of string enum names. Applies to both attestation_type in EventAttestationObserved (line 254) and reason in EventValidatorJailed (line 632). If the API expects string names for these enums, add a matching serialize_with helper (or extend the enum_i32 module to provide one), similar to the as_str module pattern used for other fields.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/proto-build-injective/src/transformers.rs (2)
163-288: Consider extracting common field transformation logic.Multiple transformer functions (
allow_serde_int_as_str,allow_serde_enum_str_or_int,serde_alias_id_with_uppercased,serde_alias_is_perpetual) share identical boilerplate for iterating fields and rebuilding the struct. A helper could reduce duplication.♻️ Optional: Extract a field transformation helper
fn transform_struct_fields<F>(s: ItemStruct, f: F) -> ItemStruct where F: FnMut(syn::Field) -> syn::Field, { let fields_vec: Vec<syn::Field> = s.fields.clone().into_iter().map(f).collect(); let fields_named: syn::FieldsNamed = parse_quote! { { #(`#fields_vec`,)* } }; syn::ItemStruct { fields: syn::Fields::Named(fields_named), ..s } } // Usage: pub fn serde_alias_is_perpetual(s: ItemStruct) -> ItemStruct { transform_struct_fields(s, |mut field| { if let Some(ident) = &field.ident { if ident == "is_perpetual" { let serde_alias: syn::Attribute = parse_quote! { #[serde(alias = "isPerpetual")] }; field.attrs.push(serde_alias); } } field }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/proto-build-injective/src/transformers.rs` around lines 163 - 288, Several transformer functions (allow_serde_int_as_str, allow_serde_enum_str_or_int, serde_alias_id_with_uppercased, serde_alias_is_perpetual) duplicate the same fields iteration and struct rebuild boilerplate; extract a helper like transform_struct_fields(s: ItemStruct, f: impl FnMut(syn::Field) -> syn::Field) -> ItemStruct that clones fields, maps the provided closure over each field, rebuilds syn::FieldsNamed and returns a new ItemStruct, then refactor each transformer to call this helper and move the per-field mutation logic (adding serde attributes, checking types/idents, etc.) into the closure (use field.attrs.push or append as appropriate) so the core iteration/reconstruction is shared.
264-288: Consider generalizing to handle allis_*fields.This function only handles
is_perpetual. If otheris_*fields may need similar camelCase aliasing in the future, consider generalizing:♻️ Optional: Generalize to handle any `is_*` field
-pub fn serde_alias_is_perpetual(s: ItemStruct) -> ItemStruct { +pub fn serde_alias_is_fields(s: ItemStruct) -> ItemStruct { let fields_vec = s .fields .clone() .into_iter() .map(|mut field| { if let Some(ident) = &field.ident { - if ident == "is_perpetual" { + let ident_str = ident.to_string(); + if ident_str.starts_with("is_") { + // Convert is_foo to isFoo + let camel = format!("is{}", ident_str[3..].to_upper_camel_case()); let serde_alias: syn::Attribute = parse_quote! { - #[serde(alias = "isPerpetual")] + #[serde(alias = `#camel`)] }; field.attrs.append(&mut vec![serde_alias]); } } field }) .collect::<Vec<syn::Field>>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/proto-build-injective/src/transformers.rs` around lines 264 - 288, The serde_alias_is_perpetual function currently only adds a #[serde(alias = "isPerpetual")] for the field named "is_perpetual"; change it to handle any field whose identifier starts with "is_" by computing a camelCase alias from the snake_case ident (e.g. "is_perpetual_option" -> "isPerpetualOption") and adding #[serde(alias = "<camelCase>"] to that field's attrs. Update the mapping in serde_alias_is_perpetual (where fields_vec is built and the code checks field.ident) to: check ident.as_ref().to_string().starts_with("is_"), compute the alias string by splitting on '_' and capitalizing subsequent segments, skip adding the alias if it already exists, and append the new serde alias attribute to field.attrs; leave other code (fields_named, fields, and returning syn::ItemStruct) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/proto-build-injective/src/transformers.rs`:
- Around line 163-288: Several transformer functions (allow_serde_int_as_str,
allow_serde_enum_str_or_int, serde_alias_id_with_uppercased,
serde_alias_is_perpetual) duplicate the same fields iteration and struct rebuild
boilerplate; extract a helper like transform_struct_fields(s: ItemStruct, f:
impl FnMut(syn::Field) -> syn::Field) -> ItemStruct that clones fields, maps the
provided closure over each field, rebuilds syn::FieldsNamed and returns a new
ItemStruct, then refactor each transformer to call this helper and move the
per-field mutation logic (adding serde attributes, checking types/idents, etc.)
into the closure (use field.attrs.push or append as appropriate) so the core
iteration/reconstruction is shared.
- Around line 264-288: The serde_alias_is_perpetual function currently only adds
a #[serde(alias = "isPerpetual")] for the field named "is_perpetual"; change it
to handle any field whose identifier starts with "is_" by computing a camelCase
alias from the snake_case ident (e.g. "is_perpetual_option" ->
"isPerpetualOption") and adding #[serde(alias = "<camelCase>"] to that field's
attrs. Update the mapping in serde_alias_is_perpetual (where fields_vec is built
and the code checks field.ident) to: check
ident.as_ref().to_string().starts_with("is_"), compute the alias string by
splitting on '_' and capitalizing subsequent segments, skip adding the alias if
it already exists, and append the new serde alias attribute to field.attrs;
leave other code (fields_named, fields, and returning syn::ItemStruct)
unchanged.
1 participant
Summary by CodeRabbit
New Features
Improvements