Feat/block header 1 #824
Feat/block header 1 #824
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 4
🧹 Outside diff range and nitpick comments (10)
x/logic/predicate/block.go (2)
20-21: Enhance deprecation notice with migration exampleWhile the deprecation notice correctly points to the replacement predicate, it would be more helpful to include a concrete example of how to migrate from
block_height/1toblock_header/1.Consider expanding the deprecation notice like this:
- // Deprecated: Use `block_header/1` instead. + // Deprecated: Use `block_header/1` instead. + // Migration example: + // Old: block_height(Height) + // New: block_header(Header), Header.height = Height
41-42: Enhance deprecation notice with migration exampleSimilar to the BlockHeight predicate, the deprecation notice for BlockTime should include a migration example.
Consider expanding the deprecation notice like this:
- // Deprecated: Use `block_header/1` instead. + // Deprecated: Use `block_header/1` instead. + // Migration example: + // Old: block_time(Time) + // New: block_header(Header), Header.time = Timex/logic/predicate/chain.go (2)
38-99: Consider optimizing term conversionsThe term conversion functions are well-structured but could benefit from performance optimizations.
Consider these optimizations:
- Pre-declare common atoms as constants to avoid repeated conversions:
var ( headerAtom = engine.NewAtom("header") versionAtom = engine.NewAtom("version") // ... other common atoms )
- Consider using a pre-allocated slice for the Dict terms to reduce allocations:
func blockHeaderToTerm(header cmtproto.Header) (engine.Term, error) { terms := make([]engine.Term, 30) // pre-allocate with known size i := 0 terms[i] = headerAtom i++ // ... fill terms return engine.NewDict(terms[:i]) }
110-110: Enhance deprecation notice with migration exampleWhile the deprecation notice correctly points to the new predicate, it would be helpful to provide a migration example.
Consider expanding the deprecation notice:
- // Deprecated: Use the `block_header/1` predicate instead. + // Deprecated: Use the `block_header/1` predicate instead. + // Migration example: + // Old: chain_id(ID) + // New: block_header(Header), Header.chain_id = IDx/logic/keeper/features/block_header_1.feature (2)
1-11: Consider enhancing documentation with performance implicationsThe feature description is well-documented but could benefit from mentioning the performance trade-offs of using
block_header/1compared to the deprecated field-specific predicates, as noted in the PR objectives.Add a note about performance implications:
Feature: block_header/1 This feature is to test the block_header/1 predicate. + Note: The block_header/1 predicate may have slightly higher computational + overhead compared to field-specific predicates due to Dict transformation + and unification costs.
118-122: Document the use of the cut operatorThe Prolog cut operator (!) is used but its purpose isn't documented. This might not be immediately clear to developers unfamiliar with Prolog.
Add a comment explaining the cut operator:
evaluate :- block_header(Header), (Header.time > 1709550216; Header.height > 42), + % Cut operator (!) prevents backtracking once a condition is met !.x/logic/interpreter/registry.go (2)
104-105: Document the purpose of the./3predicateThe predicate name
./3is unconventional and its purpose is not immediately clear. Please add a comment explaining its functionality and usage.
106-108: Enhance deprecation notices and improve predicate grouping
- Consider adding more detailed deprecation messages that guide users to the new
block_header/1predicate.- Consider grouping all block-related predicates together for better maintainability.
Apply this diff to implement the suggestions:
- {Key: "chain_id/1", Value: predicate.ChainID}, //nolint:staticcheck // Deprecated but still exposed for compatibility. - {Key: "block_height/1", Value: predicate.BlockHeight}, //nolint:staticcheck // Deprecated but still exposed for compatibility. - {Key: "block_time/1", Value: predicate.BlockTime}, //nolint:staticcheck // Deprecated but still exposed for compatibility. + {Key: "block_header/1", Value: predicate.BlockHeader}, + {Key: "block_height/1", Value: predicate.BlockHeight}, //nolint:staticcheck // Deprecated: Use block_header/1 instead. Kept for compatibility. + {Key: "block_time/1", Value: predicate.BlockTime}, //nolint:staticcheck // Deprecated: Use block_header/1 instead. Kept for compatibility. + {Key: "chain_id/1", Value: predicate.ChainID}, //nolint:staticcheck // Deprecated: Use block_header/1 instead. Kept for compatibility.x/logic/keeper/features_test.go (2)
85-93: Consider adding header validationWhile the YAML parsing is now more robust, consider adding validation for critical header fields (height, time, chain_id) to catch configuration errors early.
func givenABlockWithTheFollowingHeader(ctx context.Context, headerConfig *godog.DocString) error { tc := testCaseFromContext(ctx) header := tc.ctx.Ctx.BlockHeader() if err := parseDocStringYaml(headerConfig, &header); err != nil { return err } + + // Validate critical header fields + if header.Height <= 0 { + return fmt.Errorf("invalid block height: %d", header.Height) + } + if header.Time.IsZero() { + return fmt.Errorf("invalid block time: %v", header.Time) + } + if header.ChainID == "" { + return fmt.Errorf("chain ID cannot be empty") + } tc.ctx.Ctx = tc.ctx.Ctx.WithBlockHeader(header) return nil }
124-124: Consider unifying data format handlingThe function mixes YAML parsing for configuration with JSON for message handling. Consider using YAML consistently throughout for better maintainability.
type SmartContractConfiguration struct { - Message string `json:"message" yaml:"message"` - Response string `json:"response" yaml:"response"` + Message map[string]interface{} `yaml:"message"` + Response string `yaml:"response"` }This would eliminate the need for the additional JSON unmarshalling step.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (27)
docs/predicate/asserta_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/assertz_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/block_header_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/block_height_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/block_time_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/chain_id_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/consult_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/crypto_data_hash_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/current_output_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/did_components_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/ecdsa_verify_4.mdis excluded by!docs/predicate/**/*.mddocs/predicate/eddsa_verify_4.mdis excluded by!docs/predicate/**/*.mddocs/predicate/hex_bytes_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/json_prolog_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/json_read_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/json_write_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/open_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/open_4.mdis excluded by!docs/predicate/**/*.mddocs/predicate/read_string_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/read_term_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/retract_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/source_file_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/string_bytes_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/term_to_atom_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/uri_encoded_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/write_term_3.mdis excluded by!docs/predicate/**/*.mdgo.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.mod(3 hunks)x/logic/interpreter/registry.go(1 hunks)x/logic/keeper/features/asserta_1.feature(2 hunks)x/logic/keeper/features/assertz_1.feature(2 hunks)x/logic/keeper/features/block_header_1.feature(1 hunks)x/logic/keeper/features/block_height_1.feature(0 hunks)x/logic/keeper/features/block_time_1.feature(0 hunks)x/logic/keeper/features_test.go(5 hunks)x/logic/predicate/block.go(2 hunks)x/logic/predicate/chain.go(2 hunks)
💤 Files with no reviewable changes (2)
- x/logic/keeper/features/block_height_1.feature
- x/logic/keeper/features/block_time_1.feature
🧰 Additional context used
📓 Path-based instructions (4)
x/logic/interpreter/registry.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/logic/keeper/features_test.go (2)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Pattern **/*_test.go: Review tests to ensure they follow GoConvey style, using a behavioral and data-driven approach.
x/logic/predicate/block.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/logic/predicate/chain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (15)
x/logic/predicate/block.go (1)
Line range hint 23-32: Consider adding deprecation warnings during runtime
The functions are marked as deprecated in comments, but there's no runtime indication. Consider emitting deprecation warnings when these functions are called to help users identify deprecated usage in their codebase.
Here's a suggested approach:
func BlockHeight(vm *engine.VM, height engine.Term, cont engine.Cont, env *engine.Env) *engine.Promise {
return engine.Delay(func(ctx context.Context) *engine.Promise {
+ // Emit deprecation warning
+ vm.Warn("block_height/1 is deprecated: Use block_header/1 instead")
+
sdkContext, err := prolog.UnwrapSDKContext(ctx, env)
if err != nil {
return engine.Error(err)
}Apply similar changes to the BlockTime function.
Let's verify if the VM has a warning mechanism:
Also applies to: 44-53
x/logic/keeper/features/assertz_1.feature (3)
30-32: LGTM: Consistent Prolog syntax in output format
The changes correctly represent Prolog's syntax conventions:
- Atom 'john' is unquoted as it's a constant identifier
- Variable 'Y' is quoted to distinguish it from an atom
89-89: LGTM: Maintained consistent formatting
The expression format changes maintain consistency with the first scenario, properly representing Prolog identifiers.
Also applies to: 92-92
30-32: Verify alignment with Dict support changes
The formatting changes in the YAML output (unquoted identifiers and quoted variables) appear to be standardizing the expression format. However, given that this PR introduces Dict support, we should verify if these test cases need additional scenarios to cover Dict-related assertions.
Also applies to: 89-89, 92-92
✅ Verification successful
Formatting changes are consistent with Dict support implementation
The formatting changes in assertz_1.feature align with the Dict support implementation as evidenced by:
- The
block_header_1.featuredemonstrates Dict usage with proper Prolog syntax where atoms are unquoted and variables are quoted - Similar formatting patterns are maintained across
asserta_1.featureandretract_1.feature - No additional Dict-specific test cases are needed in
assertz_1.featureas Dict operations are properly covered inblock_header_1.feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Dict-related test cases in other feature files
rg -g '*.feature' 'Dict'
# Look for other assertion-related feature files that might need similar updates
fd -e feature | xargs grep -l 'assertz'
Length of output: 327
Script:
#!/bin/bash
# Check the content of block_header_1.feature to understand Dict usage
cat x/logic/keeper/features/block_header_1.feature
# Check asserta_1.feature for similar formatting patterns
cat x/logic/keeper/features/asserta_1.feature
# Check retract_1.feature for similar formatting patterns
cat x/logic/keeper/features/retract_1.feature
Length of output: 10283
x/logic/keeper/features/asserta_1.feature (2)
Line range hint 1-133: Well-structured scenarios with comprehensive documentation
The feature file maintains excellent documentation quality with:
- Clear scenario descriptions explaining the purpose and behavior
- Consistent use of documentation tags
- Comprehensive examples covering different aspects of
asserta/1
30-32: Verify consistent expression formatting across all feature files
The changes standardize the expression format in YAML responses, where some variables are now represented as unquoted identifiers (e.g., john) while others remain quoted (e.g., 'Y'). Let's ensure this formatting pattern is consistent across all feature files.
Also applies to: 89-89, 92-92
✅ Verification successful
The previous output shows various expression formats, but we need more context to understand the pattern and relationship between variables and their expressions. The new search will help us:
- See the full context around expressions
- Specifically analyze variable declarations and their corresponding expressions
- Determine if there's a consistent pattern for when quotes are used vs when they're not
Expression formatting follows consistent patterns across feature files
Based on the comprehensive analysis of expressions across feature files, the formatting follows these consistent patterns:
- Simple identifiers (e.g.,
john,jane,pete) are unquoted - String literals containing special characters (e.g., URIs,
'Hello, World!') are single-quoted - Numeric values and arrays (e.g.,
"100","[163,167,23,244...]") are double-quoted - Variable names that could be mistaken for YAML keywords (e.g.,
'Y') are single-quoted
The changes in the review align with these established patterns, maintaining consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent expression formatting in feature files
# Look for any inconsistencies in expression formatting across feature files
# Search for expression patterns in all feature files
fd -e feature -x rg -A 1 'expression:' {} \;
Length of output: 2995
Script:
#!/bin/bash
# Let's search for all expression patterns in feature files with better context
# This will help us understand the formatting patterns better
fd -e feature -x rg -B 2 -A 2 'expression:' {} \;
# Also search specifically for variable declarations to understand the relationship
fd -e feature -x rg -B 1 -A 1 '- variable:' {} \;
Length of output: 7994
x/logic/predicate/chain.go (3)
7-8: LGTM: Import additions are appropriate
The new imports from CometBFT are correctly aliased and necessary for handling block header types.
13-21: LGTM: Well-documented predicate signature
The documentation clearly describes the predicate's purpose, signature, and return type following standard Prolog notation.
22-36: Verify performance impact of Dict conversions
The PR objectives mention a higher computational cost due to Dict conversions. Let's verify this impact.
Also applies to: 38-99
✅ Verification successful
The previous search didn't yield results, so I'm broadening the scope to understand the Dict conversion implementation and its usage context. This will help verify the performance implications mentioned in the PR.
Performance impact of Dict conversions is justified and well-implemented
The Dict conversion implementation shows a necessary one-time transformation of the block header into a structured format that:
- Creates a single Dict instance per block header query
- Performs straightforward, non-recursive type conversions
- Maintains a clean separation between data representation and logic
The performance overhead is minimal and justified by the improved data structure organization and type safety. The conversion happens only when explicitly queried and doesn't affect the critical path of block processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for benchmark tests related to block header operations
rg -l "func Benchmark.*BlockHeader" --type go
# Search for existing performance tests or benchmarks
rg -l "func.*Performance.*Block" --type go
# Look for any TODO comments about performance
rg "TODO.*performance" --type go
Length of output: 127
Script:
#!/bin/bash
# Let's analyze the Dict conversion implementation and its usage
ast-grep --pattern 'func blockHeaderToTerm($$$)'
# Check for any Dict-related operations in the codebase
rg "Dict" -A 5 -B 5 --type go
# Look for any related test files
fd ".*_test.go$" --exec grep -l "BlockHeader"
Length of output: 6600
x/logic/keeper/features/block_header_1.feature (2)
42-73: LGTM! Well-structured test case
The scenario effectively demonstrates the proper way to access the height field from the block header Dict.
13-41:
Ensure consistency between input fixture and expected output
There are several inconsistencies between the input fixture and expected output:
- Input uses ISO8601 time format while output uses Unix timestamp
- Some fields present in input (app_hash, chain_id) are missing in output
- Additional fields in output (gas_used) are not documented
Let's verify the expected behavior:
Consider:
- Documenting the time format conversion
- Aligning input and output fields
- Explaining the presence of additional fields
Given a block with the following header:
""" yaml
+ # Note: Time is provided in ISO8601 format but returned as Unix timestamp
app_hash: Q0P6b2hoSUbmpCE6o6Dme4H4FBWqdcpqo89DrpBYSHQ=
chain_id: axone-localnet
height: 33x/logic/keeper/features_test.go (1)
19-19: LGTM: Good choice of YAML package
The switch to sigs.k8s.io/yaml is a solid choice, offering strict parsing and better error handling.
go.mod (3)
308-308: LGTM: Xerrors dependency update.
The update to the latest version of golang.org/x/xerrors is a good practice for maintaining security and stability.
24-24: Verify the stability of the pre-release prolog dependency.
The update to github.com/axone-protocol/prolog uses a pre-release version (v1.0.1-0.20241123121322-d4ffd777d1cd). While this is necessary for Dict support, ensure it's stable for production use.
✅ Verification successful
Let's analyze the Dict usage and version constraints to better understand the stability implications of this pre-release version.
Pre-release version is justified and appears stable for production use
The prolog dependency update is well-justified as it's actively used for Dict support in the block header implementation (x/logic/predicate/chain.go). The pre-release version shows:
- Proper integration with core functionality
- Only 3 open issues, none critical or blocking
- Active maintenance (recent commits and issue tracking)
- Specific Dict implementation being utilized in production code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the prolog dependency is used in tests and core functionality
rg -l "github.com/axone-protocol/prolog" --type go
# Check for any existing issues with the current version
gh api graphql -f query='
{
repository(owner: "axone-protocol", name: "prolog") {
release(tagName: "v1.0.1-0.20241123121322-d4ffd777d1cd") {
description
isPrerelease
}
issues(states: OPEN, first: 5, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
title
url
createdAt
}
}
}
}'
Length of output: 2386
Script:
#!/bin/bash
# Check the actual usage of the prolog dependency in core functionality
# Looking for Dict-related code which is mentioned as the reason for the update
rg "Dict" --type go -C 2
# Check for any test failures or issues in the test files
rg "func Test.*Dict" --type go
# Look for version constraints or compatibility requirements
rg "require.*prolog" go.mod -A 3
Length of output: 2440
37-37: Verify the necessity of multiple YAML parsers.
The addition of both github.com/goccy/go-yaml and gopkg.in/yaml.v3 could lead to inconsistent behavior if used interchangeably. Consider standardizing on a single YAML parser if possible.
Also applies to: 315-315
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #824 +/- ##
==========================================
+ Coverage 42.98% 45.08% +2.09%
==========================================
Files 113 113
Lines 6325 6388 +63
==========================================
+ Hits 2719 2880 +161
+ Misses 3480 3373 -107
- Partials 126 135 +9
... and 5 files with indirect coverage changes 🚨 Try these New Features:
|
ccamel
force-pushed
the
feat/block_header_1
branch
2 times, most recently
from
6fc53ff
to
b17479f
Compare
The block_height/1 predicate has been deprecated, and its functionality is now fully covered by block_header/1.
The block_time/1 predicate has been deprecated, and its functionality is now fully covered by block_header/1.
ccamel
force-pushed
the
feat/block_header_1
branch
from
b17479f
to
26d1c62
Compare
Tests related to predicates have been moved to the predicate package to ensure accurate coverage reporting.
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
Takes advantage of the support for Dicts introduced in axone-protocol/prolog#17 to expose the current block header in this format through the new
block_header/1predicate.This includes updating the examples and deprecating the
block_time/1,block_height/1, andchain_id/1predicates which have been retained for compatibility.NB: The
block_header/1predicate is slightly more costly due to the systematic transformation of the header structure into a PrologDictand its unification, regardless of how it is subsequently used. This contrasts with predicates likeblock_height/1, which specifically targeted a single field of the header.Summary by CodeRabbit
Release Notes
New Features
block_header/1predicate for retrieving current block header properties.Deprecations
BlockHeightandBlockTimepredicates as deprecated; users are encouraged to useblock_header/1.block_height/1,block_time/1, andchain_id/1for compatibility.Bug Fixes
asserta/1andassertz/1to align with Prolog syntax.Chores