feat(x/bank): Replace regex parsing of denom validation to generated parsing #19511
Conversation
WalkthroughWalkthroughThe recent update focuses on enhancing the efficiency and reliability of coin denomination validation and parsing within the Cosmos SDK. By replacing regex parsing with direct matching methods and introducing a new codec and comparison method, the changes aim to optimize runtime performance and improve the codebase's maintainability. Changes
Assessment against linked issues
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (6)
- types/coin.go (2 hunks)
- types/coin_regex.go (1 hunks)
- types/coin_regex.rl (1 hunks)
- types/coin_test.go (1 hunks)
- types/dec_coin.go (1 hunks)
- types/dec_coin_test.go (1 hunks)
Files not summarized due to errors (1)
- types/coin_regex.go: Error: Message exceeds token limit
Additional comments: 54
types/coin_regex.rl (2)
- 3-21: The
MatchDenomfunction is well-implemented using Ragel, providing a more efficient alternative to regex-based parsing for coin denominations. The use of character classes and patterns is clear and concise. However, it's important to ensure comprehensive test coverage for various valid and invalid denomination strings to validate the correctness of this parsing logic.- 24-66: The
MatchDecCoinfunction introduces a more complex parsing logic to handle decimal coin amounts and denominations. The use of actions to mark the start and end of amounts and denominations is a good practice, allowing for precise extraction. However, similar toMatchDenom, it's crucial to verify the correctness through extensive testing, especially for edge cases and invalid inputs.types/dec_coin.go (1)
- 634-642: The update to
ParseDecCointo useMatchDecCoinfor parsing is a significant improvement in terms of performance. By avoiding regex and using specific byte positions for amount and denomination extraction, the function becomes more efficient. Ensure that this new parsing logic is thoroughly tested, particularly for handling edge cases and ensuring that the extracted positions accurately reflect the intended parts of the input string.types/coin.go (1)
- 846-852: The replacement of regex-based validation with a Ragel-generated function (
MatchDenom) in theValidateDenomfunction is a significant change aimed at improving performance by reducing runtime overhead. This approach aligns well with the PR's objectives of enhancing efficiency and maintainability by moving away from regex for denom validation.
- Performance: The shift to a compile-time generated parsing method should indeed offer substantial performance improvements over the regex-based approach, especially in scenarios where denom validation is a frequent operation.
- Maintainability: While the performance benefits are clear, it's important to consider the maintainability of the Ragel-generated code. Generated code can sometimes be harder to read or modify than hand-written code, depending on how it's structured and documented. It would be beneficial to ensure that there's sufficient documentation on how to regenerate this code if the validation rules need to change in the future.
Overall, this change appears to be a positive step towards optimizing the Cosmos SDK's performance and codebase clarity. However, ensuring the maintainability of the Ragel-generated code is crucial for future modifications and understanding of the validation logic.
types/coin_regex.go (2)
- 5-448: The
MatchDenomfunction is well-structured and follows the expected pattern for a Ragel-generated state machine. It's designed to validate coin denominations efficiently. However, it's crucial to ensure that the generated code aligns with the intended patterns for denominations. Since this is machine-generated, it's assumed to be correct based on the Ragel specification provided. The function returns a boolean indicating whether the input matches the denomination pattern, which is a straightforward and expected behavior.- 451-963: The
MatchDecCoinfunction is designed to parse decimal coin amounts and extract the amount and denomination parts. It's more complex thanMatchDenomdue to the need to capture specific parts of the input. The function returns four values:amountStart,amountEnd,denomEnd, andisValid, which are used to extract the amount and denomination from the input.
- The use of
-1as the initial value foramountStart,amountEnd, anddenomEndis a good practice for indicating that these positions have not been found.- The
isValidflag is correctly set based on the state machine's final state, ensuring that only valid decimal coin strings are accepted.- The adjustments made in the EOF actions (e.g., adjusting
denomEndto exclude space if present) are thoughtful and necessary for correct parsing.Overall, the function appears to be correctly implemented according to the Ragel specification. It's important to have comprehensive tests to ensure that all edge cases are handled correctly, especially given the complexity of parsing decimal coin amounts.
types/dec_coin_test.go (1)
- 380-382: The added test case for parsing decimal coins with a value of 5.5 atoms is correctly implemented and follows the established testing pattern in the file. It's good to see that the test case explicitly checks for the expected outcome, ensuring that the parsing logic handles decimal values correctly. This test is crucial for validating the changes made to the parsing logic in the Cosmos SDK.
types/coin_test.go (47)
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The package declaration and imports are correctly structured, adhering to Go conventions. It's good practice to group standard library imports separately from third-party libraries, which is followed here.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [11-11]
The global variables
testDenom1andtestDenom2are well-named, indicating their purpose as denominations for test coins. This enhances readability and maintainability of the test suite.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13-13]
The
coinTestSuitestruct is well-organized, containing fields for various test coins and a field for empty coins. This setup facilitates the reuse of test data across different test cases, promoting the DRY (Don't Repeat Yourself) principle.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [15-15]
The
TestCoinTestSuitefunction correctly initializes the test suite using thesuite.Runfunction from thetestify/suitepackage. This approach leverages the setup and teardown hooks provided by the testify suite, allowing for more organized and manageable test code.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [17-17]
The
SetupSuitemethod is efficiently setting up test coins for use in the suite's test cases. The use ofmath.NewIntandsdk.NewCoinensures that the test data is accurately represented and easily modifiable.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [21-21]
The
TestCoinmethod correctly tests the creation of coins with various denominations and amounts, including negative amounts to ensure that the coin creation logic correctly panics in such cases. This is crucial for maintaining the integrity of coin operations within the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [23-23]
The
TestCoin_Stringmethod effectively tests the string representation of a coin, ensuring that it matches the expected format. This is important for user interfaces and logging, where a clear and accurate representation of coins is necessary.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [25-25]
The
TestIsEqualCoinmethod thoroughly tests the equality comparison between coins, covering various scenarios including different denominations and amounts. This ensures the reliability of coin comparisons, which are fundamental to many operations in the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [27-27]
The
TestCoinIsValidmethod tests the validity of coins with various denominations, including edge cases like very long denominations and special characters. This comprehensive testing is essential for ensuring that only valid coins are accepted by the system.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]
The
TestCoinsDenomsmethod correctly tests the extraction of denominations from a set of coins, ensuring that the functionality works as expected even in edge cases. This is important for operations that need to work with the denominations of coins directly.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [31-31]
The
TestAddCoinmethod provides a thorough test of the addition operation for coins, including checks for overflows. This is crucial for ensuring the accuracy and safety of coin arithmetic operations within the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [33-33]
The
TestAddCoinAmountmethod tests the addition of a raw amount to a coin, covering both positive and zero addition cases. This ensures that coin amounts can be accurately and safely modified.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The
TestSubCoinmethod tests the subtraction operation between coins, including cases that should panic due to invalid operations. This ensures the robustness and safety of coin subtraction within the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [37-37]
The
TestSubCoinAmountmethod tests the subtraction of a raw amount from a coin, including cases that should panic due to resulting in negative amounts. This is important for maintaining the integrity of coin values.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [39-39]
The
TestMulIntCoinsmethod tests the multiplication of a set of coins by an integer, including a case that should panic due to overflow. This ensures that coin values can be accurately scaled up while maintaining system integrity.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [41-41]
The
TestQuoIntCoinsmethod tests the division of a set of coins by an integer, including cases that should panic due to division by zero. This ensures that coin values can be accurately scaled down in a safe manner.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-43]
The
TestIsGTCoinmethod tests the greater-than comparison between coins, including cases that should panic due to invalid comparisons. This ensures the reliability of coin comparisons, which are essential for decision-making logic in the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [45-45]
The
TestIsGTECoinmethod tests the greater-than-or-equal-to comparison between coins, including cases that should panic due to invalid comparisons. This ensures the accuracy of coin comparisons, supporting various operations and logic within the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [47-47]
The
TestIsLTECoinmethod tests the less-than-or-equal-to comparison between coins, including cases that should panic due to invalid comparisons. This is important for ensuring the correctness of comparisons, which are fundamental to many SDK operations.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [49-49]
The
TestIsLTCoinmethod tests the less-than comparison between coins, including cases that should panic due to invalid comparisons. This ensures the robustness of coin comparison logic, which is crucial for various SDK functionalities.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [51-51]
The
TestCoinIsZeromethod correctly tests whether a coin's amount is zero, covering both true and false cases. This functionality is essential for operations that need to distinguish between zero and non-zero coin amounts.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [53-53]
The
TestCoinIsNilmethod tests the nil-check functionality for coins, ensuring that both uninitialized coins and coins with a denomination but no amount are considered nil. This is important for handling uninitialized or partially initialized coins correctly.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [55-55]
The
TestFilteredZeroCoinsmethod tests the filtering of zero-amount coins from a set, ensuring that only coins with non-zero amounts are retained. This is crucial for operations that should ignore zero-amount coins.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [57-57]
The
TestCoins_Stringmethod tests the string representation of a set of coins, ensuring that it matches the expected format. This is important for displaying coin sets in user interfaces and logs.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [59-59]
The
TestIsZeroCoinsmethod tests whether a set of coins contains only zero-amount coins, covering various scenarios. This functionality is essential for operations that need to treat zero-amount coin sets differently from non-zero ones.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [61-61]
The
TestEqualCoinsmethod thoroughly tests the equality comparison between sets of coins, ensuring that the comparison logic is accurate and reliable. This is fundamental to many operations within the SDK that rely on comparing coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [63-63]
The
TestAddCoinsmethod tests the addition of two sets of coins, including cases that should result in zero coins being removed from the result. This ensures the accuracy and safety of adding coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [65-65]
The
TestCoinsAddCoalescesDuplicateDenominationsmethod tests the addition of coin sets with duplicate denominations, ensuring that duplicates are correctly coalesced. This is crucial for maintaining the integrity of coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [67-67]
The
TestSubCoinsmethod tests the subtraction of one set of coins from another, including cases that should panic due to invalid operations. This ensures the robustness and safety of coin set subtraction within the SDK.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [69-69]
The
TestSafeSubCoinmethod tests the safe subtraction of one coin from another, including cases that should result in an error due to negative results. This is important for ensuring that coin subtraction is performed safely and accurately.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [71-71]
The
TestCoins_Validatemethod tests the validation logic for sets of coins, including various invalid scenarios. This comprehensive testing is essential for ensuring that only valid coin sets are accepted by the system.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [73-73]
The
TestMinMaxmethod tests the calculation of the minimum and maximum between two sets of coins, ensuring that the functionality works as expected. This is important for operations that need to compare coin sets to determine bounds.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [75-75]
The
TestCoinsGTmethod tests the greater-than comparison between sets of coins, ensuring that the comparison logic is accurate. This is crucial for decision-making logic within the SDK that relies on comparing coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [77-77]
The
TestCoinsLTmethod tests the less-than comparison between sets of coins, ensuring that the comparison logic is accurate. This is important for various SDK operations that depend on comparing coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [79-79]
The
TestCoinsLTEmethod tests the less-than-or-equal-to comparison between sets of coins, ensuring that the comparison logic is accurate. This supports the correctness of operations that rely on comparing coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [81-81]
The
TestParseCoinsmethod tests the parsing of coin strings into sets of coins, covering various scenarios including invalid inputs. This ensures that coin strings can be accurately and safely converted into coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [83-83]
The
TestSortCoinsmethod tests the sorting functionality for sets of coins, ensuring that coins are correctly sorted and that invalid coins are handled appropriately. This is crucial for maintaining the order and integrity of coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [85-85]
The
TestSearchmethod tests the search functionality within sets of coins, ensuring that coins can be accurately found or determined to be absent. This supports operations that need to query coin sets for specific denominations.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [87-87]
The
TestCoinsIsAnyGTEmethod tests whether any coin in one set is greater than or equal to any coin in another set, covering various scenarios. This is important for operations that need to compare coin sets in a non-strict manner.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [89-89]
The
TestCoinsIsAllGTmethod tests whether all coins in one set are strictly greater than all coins in another set, ensuring the accuracy of this comparison logic. This supports the correctness of operations that rely on strict comparisons between coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [91-91]
The
TestCoinsIsAllGTEmethod tests whether all coins in one set are greater than or equal to all coins in another set, ensuring the accuracy of this comparison logic. This is crucial for operations that rely on non-strict comparisons between coin sets.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [93-93]
The
TestNewCoinsmethod tests the creation of new coin sets, including cases that should panic due to invalid inputs. This ensures that coin sets are correctly initialized and validated upon creation.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [95-95]
The
TestCoinsIsAnyGTmethod tests whether any coin in one set is strictly greater than any coin in another set, covering various scenarios. This is important for operations that need to compare coin sets in a specific manner.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [97-97]
The
TestCoinsIsAnyNilmethod tests whether any coin in a set is considered nil, ensuring that sets with uninitialized or partially initialized coins are correctly identified. This is crucial for handling coin sets safely.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [99-99]
The
TestMarshalJSONCoinsmethod tests the JSON marshaling and unmarshaling of coin sets, ensuring that coin sets can be accurately represented in JSON format and reconstructed from it. This is important for data serialization and communication.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [101-101]
The
TestCoinValidatemethod tests the validation logic for individual coins, including various invalid scenarios. This comprehensive testing is essential for ensuring that only valid coins are accepted by the system.
- 106-111: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [103-103]
The
TestCoinAminoEncodingmethod tests the Amino encoding and decoding of individual coins, ensuring that coins can be accurately serialized and deserialized using the Amino codec. This is important for data persistence and communication.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/coin_regex.go (1 hunks)
Files not summarized due to errors (1)
- types/coin_regex.go: Error: Message exceeds token limit
Additional comments: 2
types/coin_regex.go (2)
- 8-455: The
MatchDenomfunction is well-structured and follows the generated pattern from Ragel for state machine-based parsing. The function aims to validate coin denominations using a finite state machine, which is a more efficient approach compared to regex parsing. The transition arrays and logic are generated by Ragel, ensuring correctness as long as the.rlfile is correctly defined. The function returns a boolean indicating whether the input matches the denom pattern, which aligns with the PR's objective to improve performance and maintainability by replacing regex-based validation.- 458-972: The
MatchDecCoinfunction introduces a more complex state machine to parse decimal coin amounts, capturing the start and end positions of the amount and denomination. This function is a significant improvement in terms of performance and precision over regex-based parsing for decimal coins. The use of action codes to mark the start and end of the amount and to adjust the end position of the denomination is a clever use of Ragel's capabilities. However, it's crucial to ensure that the.rlfile accurately defines the expected patterns to avoid any parsing inaccuracies. The function returns the start and end positions for the amount and denomination, along with a validity flag, which is a practical approach for downstream processing.
|
I'm not familiar with Ragel, could we get a small benchmark to compare? Also, how is the code generated? Thank you! |
|
@facundomedica of course! I'll get the benchmark tomorrow and reply in the PR |
mattverse
force-pushed
the
mattverse/use-ragel
branch
from
5f24000
to
fac684e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- types/coin.go (2 hunks)
- types/coin_regex.go (1 hunks)
- types/coin_regex.rl (1 hunks)
Files not summarized due to errors (1)
- types/coin_regex.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (2)
- types/coin.go
- types/coin_regex.rl
Additional comments: 4
CHANGELOG.md (4)
- 45-45: The entry for replacing regex parsing with direct matching methods for denom validation accurately reflects the PR's main objective. It's clear and directly links to the PR, facilitating easy reference.
- 45-45: The addition of a
ValueCodecfor themath.Uinttype in collections maps is mentioned. This change seems unrelated to the main focus of the PR but is a valuable addition to the SDK. Ensure that this change has been documented in the appropriate sections of the SDK documentation for developers to understand its usage and benefits.Verification successful
The search results indicate that the documentation, specifically within the
collections/README.mdfile, has been updated to mention "math.Uint" multiple times. This suggests that the addition of aValueCodecfor themath.Uinttype in collections maps, as mentioned in the review comment, is likely documented, albeit not with the explicit use of the term "ValueCodec". Given the context and the findings, it appears that the documentation has been updated in relation to the change, but the specific terminology used in the review comment may not have been directly matched in the documentation.* 45-45: The introduction of the `IsGT` method for `types.Coin` is correctly documented. This method enhances the SDK's functionality by providing a straightforward way to compare coin values. It's important to ensure that examples of using `IsGT` are included in the SDK documentation to help developers understand its application. * 45-45: Adding the `--qrcode` flag to the `keys show` command is a user-friendly feature that enhances the usability of the SDK. This change should be highlighted in the SDK's user guide or command-line tool documentation to ensure users are aware of how to use this new feature.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation updates related to ValueCodec for math.Uint rg "ValueCodec for math.Uint" docs/Length of output: 37
Script:
#!/bin/bash # Broaden the search for documentation updates related to ValueCodec and math.Uint separately rg "ValueCodec" -or "math.Uint" docs/ # Additionally, search for any mention of ValueCodec or math.Uint in README files or developer guides that might not be in the docs/ directory rg "ValueCodec" -or "math.Uint" --glob "*README.md" --glob "*developer_guide*"Length of output: 400
|
Finished benchmarking! Bench marked using existing benchmarking test https://github.com/cosmos/cosmos-sdk/blob/main/types/bench_test.go#L18 on parsing coins. Here are the results: We can see
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (2)
- types/coin_regex.go (1 hunks)
- types/coin_regex.rl (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- types/coin_regex.rl
Additional comments: 2
types/coin_regex.go (2)
- 2-5: The file header clearly states that this is generated code and should not be edited manually. It also specifies the source file and disables certain linter warnings. This is a good practice for generated code to ensure maintainability and clarity about its generation and purpose.
- 8-15: The comments explaining the previous regex patterns provide valuable context for understanding the transition from regex to Ragel. Including a brief comment about the choice of Ragel over regex, as suggested in the existing comments, is beneficial for future maintainers.
|
I think the speed improvement is actually way higher. The IBC hashes are going to be much longer, but this isn't in the coin benchmarks |
There was a problem hiding this comment.
What is the difference between using regal over a simple go loop since we know the input. Bez mentioned that it is recommended to move away from regular expression once there is understanding of the format.
@mattverse would love to better understand the reasoning behind using regal over native golang
|
Ragel is native golang, its just code generated. I think its worth benchmarking against the for loop I put in the GH issue as well though |
|
I just benchmarked only the Now loop vs ragel: Used these denoms: var denoms = []string{
"ATM",
"AMX",
"XXX",
"BTC",
"ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2",
"ibc/498A0751C798A0D9A389AA3691123DADA57DAA4FE165D5C75894505B876BA6E4",
"uatom",
} |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- types/coin_regex.go (1 hunks)
- types/coin_regex.rl (1 hunks)
- types/dec_coin.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- types/coin_regex.go
- types/coin_regex.rl
- types/dec_coin.go
|
@facundomedica Amazing! Thanks so much 🙇 So is the claim loop shows approx 75% decrease then regel showing another 75% decrease from there? Also curious about other stats! |
| return fmt.Errorf("invalid denom: %s", denom) | ||
| if reDnm == nil || reDecCoin == nil { | ||
| // Convert the string to a byte slice as required by the Ragel-generated function. | ||
| denomBytes := []byte(denom) |
There was a problem hiding this comment.
In the future we should do internal.unsafeconvstringtobytes
That should fix the allocations in this fn
types/coin_regex.rl
Outdated
| @@ -0,0 +1,39 @@ | |||
| // Code generated by regel using `ragel -Z coin_regex.rl`. | |||
There was a problem hiding this comment.
Comment is misleading, this code wasn't generated right? It was written and then coin_regex.go was generated from it?
There was a problem hiding this comment.
The comment here goes directly to the generated code( coin_regex.go ). Changed in the most recent commit to be more clear!
|
once linting passes we can merge. Thank you spending time on this |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- types/coin.go (2 hunks)
- types/coin_regex.go (1 hunks)
- types/coin_regex.rl (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- types/coin.go
- types/coin_regex.go
- types/coin_regex.rl
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- tests/integration/bank/keeper/deterministic_test.go (1 hunks)
- tests/integration/staking/keeper/deterministic_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/staking/keeper/deterministic_test.go
* feat(x/bank): Replace regex parsing of denom validation to generated parsing (cosmos#19511) * fix: fix ragel denom validation introduced in cosmos#19511 (cosmos#19701) * fix: use loop instead of ragel (cosmos#19710) * Fix test * Fix integration test as well * Try linting * Fix lint * Fix lint * add changelog --------- Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
* feat(x/bank): Replace regex parsing of denom validation to generated parsing (cosmos#19511) * fix: fix ragel denom validation introduced in cosmos#19511 (cosmos#19701) * fix: use loop instead of ragel (cosmos#19710) * Fix test * Fix integration test as well * Try linting * Fix lint * Fix lint * add changelog --------- Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit ab4bc05)
* feat(x/bank): Replace regex parsing of denom validation to generated parsing (cosmos#19511) * fix: fix ragel denom validation introduced in cosmos#19511 (cosmos#19701) * fix: use loop instead of ragel (cosmos#19710) * Fix test * Fix integration test as well * Try linting * Fix lint * Fix lint * add changelog --------- Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit ab4bc05)
* feat(x/bank): Replace regex parsing of denom validation to generated parsing (cosmos#19511) * fix: fix ragel denom validation introduced in cosmos#19511 (cosmos#19701) * fix: use loop instead of ragel (cosmos#19710) * Fix test * Fix integration test as well * Try linting * Fix lint * Fix lint * add changelog --------- Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit ab4bc05) Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
* feat(x/bank): Replace regex parsing of denom validation to generated parsing (cosmos#19511) * fix: fix ragel denom validation introduced in cosmos#19511 (cosmos#19701) * fix: use loop instead of ragel (cosmos#19710) * Fix test * Fix integration test as well * Try linting * Fix lint * Fix lint * add changelog --------- Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit ab4bc05) Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
* feat(x/bank): Replace regex parsing of denom validation to generated parsing (cosmos#19511) * fix: fix ragel denom validation introduced in cosmos#19511 (cosmos#19701) * fix: use loop instead of ragel (cosmos#19710) * Fix test * Fix integration test as well * Try linting * Fix lint * Fix lint * add changelog --------- Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Description
Closes: #17221
Replaces Regex Usage when parsing denom for validation. Instead uses generated code for the validation using ragel( https://en.wikipedia.org/wiki/Ragel).
As linked in original issue, using regex for denom validation has a huge performance overhead, thus replacing it with direct matching methods.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
ValueCodecfor enhanced type handling in collections.IsGTfortypes.Coininstances.core/router.Serviceto improve runtime service handling.ParseDecCoinfunction for more efficient parsing of decimal coins.