Conversation
Pitasi
force-pushed
the
rewrite-json-precompile
branch
from
a7a8639 to
52ba314
Compare
Pitasi
force-pushed
the
remove-old-json-precompile
branch
from
14a5af3 to
166237c
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Pitasi
force-pushed
the
rewrite-json-precompile
branch
3 times, most recently
from
f88a273 to
89a7037
Compare
Pitasi
force-pushed
the
remove-old-json-precompile
branch
2 times, most recently
from
27db637 to
786412b
Compare
Pitasi
force-pushed
the
rewrite-json-precompile
branch
3 times, most recently
from
f818d7e to
7c39d3c
Compare
Pitasi
force-pushed
the
remove-old-json-precompile
branch
from
786412b to
e7107a0
Compare
|
Hey @Pitasi and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' file. |
📝 WalkthroughWalkthroughThis change removes nearly all granular JSON manipulation methods and related structs from the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IJsonPrecompile
Client->>IJsonPrecompile: Build(ops, data)
IJsonPrecompile-->>Client: bytes (JSON object)
Client->>IJsonPrecompile: Parse(jsonBytes, schema)
IJsonPrecompile-->>Client: bytes (ABI-encoded data)
Estimated code review effort4 (~90 minutes) Possibly related PRs
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
![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
🔭 Outside diff range comments (1)
precompiles/json/IJson.sol (1)
10-14: Remove the unused ReadKeyValue struct.This struct appears to be a remnant of the old API and is no longer used in the simplified interface.
-struct ReadKeyValue { - string key; - string valueType; - int64 decimals; -} -
🧹 Nitpick comments (1)
precompiles/json/query.go (1)
21-22: Consider using custom error types instead of fmt.Errorf.Based on codebase conventions, custom error types are preferred over
fmt.Errorffor better error handling and code clarity.Example custom error types:
type WrongArgsNumber struct { Expected int Got int } func (e WrongArgsNumber) Error() string { return fmt.Sprintf("wrong args number, expected %d, got %d", e.Expected, e.Got) } type WrongArgType struct { Position int Expected string Got string } func (e WrongArgType) Error() string { return fmt.Sprintf("wrong arg #%d, expected %s, got %s", e.Position, e.Expected, e.Got) }Also applies to: 26-27, 31-32, 35-35, 41-41, 53-54, 58-59, 63-64, 69-69
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
tests/testdata/contracts/json-user/IJson.abiis excluded by!tests/testdata/**tests/testdata/contracts/json-user/IJson.binis excluded by!**/*.bin,!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.abiis excluded by!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.binis excluded by!**/*.bin,!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.gois excluded by!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.solis excluded by!tests/testdata/**tests/testdata/contracts/json-user/remappings.txtis excluded by!tests/testdata/**
📒 Files selected for processing (12)
precompiles/json/IJson.go(3 hunks)precompiles/json/IJson.sol(1 hunks)precompiles/json/abi.json(1 hunks)precompiles/json/conversions.go(0 hunks)precompiles/json/json.go(1 hunks)precompiles/json/json_read.go(0 hunks)precompiles/json/json_test.go(0 hunks)precompiles/json/query.go(1 hunks)precompiles/json/validation.go(0 hunks)solidity/plugin-gmp-handler/src/PluginGmpHandler.sol(5 hunks)tests/cases/json_precompile.go(0 hunks)tests/cases/json_precompile_user.go(0 hunks)
🧠 Learnings (7)
📓 Common learnings
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
solidity/plugin-gmp-handler/src/PluginGmpHandler.sol (2)
Learnt from: mn13
PR: #1248
File: precompiles/async/IAsync.sol:67-73
Timestamp: 2025-02-18T11:53:28.386Z
Learning: The IAsync.sol file is an interface definition and should not include implementation details or validation logic. Interface files should only contain function signatures and event definitions.
Learnt from: mn13
PR: #1178
File: solidity/orders/src/OrderFactory.sol:78-91
Timestamp: 2025-01-21T15:35:49.044Z
Learning: In the Warden Protocol, view functions like computeOrderAddress should return values without reverting, even for unsupported cases. This maintains better composability and allows callers to handle edge cases as needed.
precompiles/json/json.go (2)
Learnt from: backsapc
PR: #975
File: precompiles/slinky/slinky.go:129-136
Timestamp: 2024-11-02T08:51:46.547Z
Learning: In the IsTransaction method, avoid returning false for unknown methods, as it suggests the method is a query. Instead, handle unknown methods by returning an error to indicate the method is unrecognized.
Learnt from: mn13
PR: #1196
File: prophet/handlers/pricepred/pricepred.go:175-236
Timestamp: 2025-01-30T08:02:40.983Z
Learning: In the Warden Protocol codebase, prefer using map-based function lookups over large switch-case blocks for metric type handling. Example implementation:
metricMap := map[MetricName]func(BacktestingMetrics) float64{
MetricType: func(m BacktestingMetrics) float64 { return m.Value },
// ...
}precompiles/json/query.go (2)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
Learnt from: Pitasi
PR: #889
File: precompiles/act/query.go:170-171
Timestamp: 2024-10-16T15:30:41.746Z
Learning: In Go code within precompile methods (e.g., in precompiles/act/query.go), prefer defining and using a custom error type (like WrongArgsNumber) instead of using fmt.Errorf directly for error messages. This enhances error handling and code clarity.
precompiles/json/abi.json (1)
Learnt from: Svetomech
PR: #1042
File: precompiles/warden/abi.json:1633-1637
Timestamp: 2024-11-14T15:15:53.718Z
Learning: In the precompiles/warden/abi.json file, the broadcastType parameter in the newSignRequest method is intentionally defined as enum BroadcastType (uint8), even though it's defined as int32 in other locations.
precompiles/json/IJson.sol (1)
Learnt from: mn13
PR: #1248
File: precompiles/async/IAsync.sol:67-73
Timestamp: 2025-02-18T11:53:28.386Z
Learning: The IAsync.sol file is an interface definition and should not include implementation details or validation logic. Interface files should only contain function signatures and event definitions.
precompiles/json/IJson.go (1)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
💤 Files with no reviewable changes (6)
- precompiles/json/validation.go
- tests/cases/json_precompile_user.go
- precompiles/json/conversions.go
- tests/cases/json_precompile.go
- precompiles/json/json_test.go
- precompiles/json/json_read.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
solidity/plugin-gmp-handler/src/PluginGmpHandler.sol (2)
Learnt from: mn13
PR: #1248
File: precompiles/async/IAsync.sol:67-73
Timestamp: 2025-02-18T11:53:28.386Z
Learning: The IAsync.sol file is an interface definition and should not include implementation details or validation logic. Interface files should only contain function signatures and event definitions.
Learnt from: mn13
PR: #1178
File: solidity/orders/src/OrderFactory.sol:78-91
Timestamp: 2025-01-21T15:35:49.044Z
Learning: In the Warden Protocol, view functions like computeOrderAddress should return values without reverting, even for unsupported cases. This maintains better composability and allows callers to handle edge cases as needed.
precompiles/json/json.go (2)
Learnt from: backsapc
PR: #975
File: precompiles/slinky/slinky.go:129-136
Timestamp: 2024-11-02T08:51:46.547Z
Learning: In the IsTransaction method, avoid returning false for unknown methods, as it suggests the method is a query. Instead, handle unknown methods by returning an error to indicate the method is unrecognized.
Learnt from: mn13
PR: #1196
File: prophet/handlers/pricepred/pricepred.go:175-236
Timestamp: 2025-01-30T08:02:40.983Z
Learning: In the Warden Protocol codebase, prefer using map-based function lookups over large switch-case blocks for metric type handling. Example implementation:
metricMap := map[MetricName]func(BacktestingMetrics) float64{
MetricType: func(m BacktestingMetrics) float64 { return m.Value },
// ...
}precompiles/json/query.go (2)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
Learnt from: Pitasi
PR: #889
File: precompiles/act/query.go:170-171
Timestamp: 2024-10-16T15:30:41.746Z
Learning: In Go code within precompile methods (e.g., in precompiles/act/query.go), prefer defining and using a custom error type (like WrongArgsNumber) instead of using fmt.Errorf directly for error messages. This enhances error handling and code clarity.
precompiles/json/abi.json (1)
Learnt from: Svetomech
PR: #1042
File: precompiles/warden/abi.json:1633-1637
Timestamp: 2024-11-14T15:15:53.718Z
Learning: In the precompiles/warden/abi.json file, the broadcastType parameter in the newSignRequest method is intentionally defined as enum BroadcastType (uint8), even though it's defined as int32 in other locations.
precompiles/json/IJson.sol (1)
Learnt from: mn13
PR: #1248
File: precompiles/async/IAsync.sol:67-73
Timestamp: 2025-02-18T11:53:28.386Z
Learning: The IAsync.sol file is an interface definition and should not include implementation details or validation logic. Interface files should only contain function signatures and event definitions.
precompiles/json/IJson.go (1)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
⏰ 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). (7)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
precompiles/json/json.go (1)
97-131: LGTM! Clean implementation with only Build and Parse methods.The removal of old JSON manipulation methods is complete and the implementation correctly handles only the new API methods as queries.
solidity/plugin-gmp-handler/src/PluginGmpHandler.sol (1)
66-90: Excellent refactoring to use the new JSON API.The migration from the old JSON API to
JsonUtils.JsonBuilderis clean and consistent. The change fromviewtopureis also correct since the function doesn't read contract state.precompiles/json/abi.json (1)
1-52: ABI correctly simplified to only Build and Parse methods.The ABI definition properly reflects the new simplified JSON precompile interface with clear parameter names.
precompiles/json/query.go (1)
15-73: Build and Parse methods implementation looks solid.The implementations correctly validate arguments, handle type conversions, and delegate to the appropriate builder/parser structs.
precompiles/json/IJson.sol (2)
31-47: Interface correctly simplified to build and parse methods.The IJson interface now provides a clean API with only the essential JSON building and parsing functionality.
75-243: JsonUtils library provides excellent JSON building functionality.The library offers a comprehensive and user-friendly API for constructing JSON with proper type safety and convenience methods.
precompiles/json/IJson.go (2)
34-34: ABI correctly reflects the simplified interface.The updated ABI now contains only the
buildandparsemethods, which aligns perfectly with the PR objective of removing the old API and retaining only these two functions.
214-243: Parse method bindings correctly implemented.The
Parsemethod bindings have been properly generated with the new signature acceptingjsonBytesandschemaparameters. All three binding variations (Caller, Session, CallerSession) are consistently updated.
Pitasi
force-pushed
the
remove-old-json-precompile
branch
from
e7107a0 to
a30450e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
precompiles/json/IJson.sol (1)
10-14: Remove unused ReadKeyValue struct.The
ReadKeyValuestruct is no longer used since all read-related methods have been removed from the interface.-struct ReadKeyValue { - string key; - string valueType; - int64 decimals; -}
🧹 Nitpick comments (2)
precompiles/json/query.go (2)
15-45: Consider using custom error types instead of fmt.Errorf.The
Buildmethod implementation is correct, but according to coding guidelines, custom error types are preferred overfmt.Errorfin precompile methods for better error handling and code clarity.Consider defining custom error types:
type WrongArgsNumber struct { Expected int Got int } func (e WrongArgsNumber) Error() string { return fmt.Sprintf("wrong args number, expected %d, got %d", e.Expected, e.Got) } type WrongArgType struct { ArgIndex int Expected string Got interface{} } func (e WrongArgType) Error() string { return fmt.Sprintf("wrong arg #%d, expected %s, got %T", e.ArgIndex, e.Expected, e.Got) }Then use them in the method:
-return nil, fmt.Errorf("wrong args number, expected 2, got %d", len(args)) +return nil, WrongArgsNumber{Expected: 2, Got: len(args)}
47-73: Parse method implementation is correct.The method properly validates arguments and delegates to the parser struct. The same custom error type suggestion applies here as well.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
tests/testdata/contracts/json-user/IJson.abiis excluded by!tests/testdata/**tests/testdata/contracts/json-user/IJson.binis excluded by!**/*.bin,!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.abiis excluded by!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.binis excluded by!**/*.bin,!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.gois excluded by!tests/testdata/**tests/testdata/contracts/json-user/JsonUser.solis excluded by!tests/testdata/**tests/testdata/contracts/json-user/remappings.txtis excluded by!tests/testdata/**
📒 Files selected for processing (12)
precompiles/json/IJson.go(3 hunks)precompiles/json/IJson.sol(1 hunks)precompiles/json/abi.json(1 hunks)precompiles/json/conversions.go(0 hunks)precompiles/json/json.go(1 hunks)precompiles/json/json_read.go(0 hunks)precompiles/json/json_test.go(0 hunks)precompiles/json/query.go(1 hunks)precompiles/json/validation.go(0 hunks)solidity/plugin-gmp-handler/src/PluginGmpHandler.sol(5 hunks)tests/cases/json_precompile.go(0 hunks)tests/cases/json_precompile_user.go(0 hunks)
🧠 Learnings (6)
📓 Common learnings
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
precompiles/json/abi.json (1)
Learnt from: Svetomech
PR: #1042
File: precompiles/warden/abi.json:1633-1637
Timestamp: 2024-11-14T15:15:53.718Z
Learning: In the precompiles/warden/abi.json file, the broadcastType parameter in the newSignRequest method is intentionally defined as enum BroadcastType (uint8), even though it's defined as int32 in other locations.
precompiles/json/json.go (1)
Learnt from: backsapc
PR: #975
File: precompiles/slinky/slinky.go:129-136
Timestamp: 2024-11-02T08:51:46.547Z
Learning: In the IsTransaction method, avoid returning false for unknown methods, as it suggests the method is a query. Instead, handle unknown methods by returning an error to indicate the method is unrecognized.
precompiles/json/IJson.sol (1)
Learnt from: mn13
PR: #1248
File: precompiles/async/IAsync.sol:67-73
Timestamp: 2025-02-18T11:53:28.386Z
Learning: The IAsync.sol file is an interface definition and should not include implementation details or validation logic. Interface files should only contain function signatures and event definitions.
precompiles/json/query.go (2)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
Learnt from: Pitasi
PR: #889
File: precompiles/act/query.go:170-171
Timestamp: 2024-10-16T15:30:41.746Z
Learning: In Go code within precompile methods (e.g., in precompiles/act/query.go), prefer defining and using a custom error type (like WrongArgsNumber) instead of using fmt.Errorf directly for error messages. This enhances error handling and code clarity.
precompiles/json/IJson.go (1)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
💤 Files with no reviewable changes (6)
- precompiles/json/validation.go
- precompiles/json/json_test.go
- precompiles/json/conversions.go
- tests/cases/json_precompile_user.go
- precompiles/json/json_read.go
- tests/cases/json_precompile.go
🚧 Files skipped from review as they are similar to previous changes (1)
- solidity/plugin-gmp-handler/src/PluginGmpHandler.sol
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
precompiles/json/abi.json (1)
Learnt from: Svetomech
PR: #1042
File: precompiles/warden/abi.json:1633-1637
Timestamp: 2024-11-14T15:15:53.718Z
Learning: In the precompiles/warden/abi.json file, the broadcastType parameter in the newSignRequest method is intentionally defined as enum BroadcastType (uint8), even though it's defined as int32 in other locations.
precompiles/json/json.go (1)
Learnt from: backsapc
PR: #975
File: precompiles/slinky/slinky.go:129-136
Timestamp: 2024-11-02T08:51:46.547Z
Learning: In the IsTransaction method, avoid returning false for unknown methods, as it suggests the method is a query. Instead, handle unknown methods by returning an error to indicate the method is unrecognized.
precompiles/json/IJson.sol (1)
Learnt from: mn13
PR: #1248
File: precompiles/async/IAsync.sol:67-73
Timestamp: 2025-02-18T11:53:28.386Z
Learning: The IAsync.sol file is an interface definition and should not include implementation details or validation logic. Interface files should only contain function signatures and event definitions.
precompiles/json/query.go (2)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
Learnt from: Pitasi
PR: #889
File: precompiles/act/query.go:170-171
Timestamp: 2024-10-16T15:30:41.746Z
Learning: In Go code within precompile methods (e.g., in precompiles/act/query.go), prefer defining and using a custom error type (like WrongArgsNumber) instead of using fmt.Errorf directly for error messages. This enhances error handling and code clarity.
precompiles/json/IJson.go (1)
Learnt from: backsapc
PR: #810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The MarkProcessed method has been removed from keychain-sdk/internal/tracker/status_tracker.go in the Go codebase, as it is no longer needed.
⏰ 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). (7)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
precompiles/json/json.go (2)
97-103: LGTM! Switch statement correctly handles only the remaining methods.The
Runmethod now only processesBuildMethodandParseMethod, which aligns with the PR objective of retaining only the new API.
125-125: Good simplification of the non-transactional method list.The
IsTransactionmethod correctly identifies bothBuildMethodandParseMethodas non-transactional (queries).precompiles/json/abi.json (1)
1-52: ABI correctly reflects the simplified JSON precompile interface.The ABI now exposes only two functions (
buildandparse) as intended, with appropriate parameter names and types.precompiles/json/query.go (1)
11-13: Methods correctly reduced to only Build and Parse.The constants now only define the two remaining methods, aligning with the simplified API.
precompiles/json/IJson.sol (2)
31-47: Interface correctly simplified to only build and parse methods.The IJson interface now exposes only the two core methods as intended by the PR.
201-201: Whitespace-only change on line 201—no action needed.The diff around line 201 shows only a blank line between the
pairoverloads foruint256andbool. There’s no functional change in the code.precompiles/json/IJson.go (2)
1-2: Auto-generated file correctly reflects the simplified interface.This file is auto-generated from the ABI and properly contains only the
BuildandParsemethod bindings.
214-243: Parse method binding correctly updated.The
Parsemethod signature has been properly updated to acceptjsonBytesandschemaparameters and returnabiEncodedData.
1 participant
This PR:
buildandpairfunctionsdepends on #1609