Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: audit #110

Merged
merged 8 commits into from
Aug 23, 2024
Merged

fix: audit #110

merged 8 commits into from
Aug 23, 2024

Conversation

beer-1
Copy link
Member

@beer-1 beer-1 commented Aug 19, 2024

Description

Closes: #XXXX


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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Improved ratio calculations in the dex modules for enhanced accuracy.
    • Added functionality to unset previously set query responses in the query modules.
  • Bug Fixes

    • Updated validation logic for collection names and token IDs to accept a single colon, broadening acceptance criteria.
  • Style

    • Minor formatting adjustments for improved readability in the JSON module tests.
  • Chores

    • Updated dependencies in the project to more stable or feature-rich versions.

@beer-1 beer-1 self-assigned this Aug 19, 2024
@beer-1 beer-1 requested a review from a team as a code owner August 19, 2024 08:18
Copy link

coderabbitai bot commented Aug 19, 2024

Walkthrough

The recent updates enhance the calculation of ratios in the dex modules by transitioning from decimal128 to decimal256, improving accuracy and accommodating larger values. The validation logic for collection names and token IDs has been simplified by switching checks from double colons (::) to single colons (:). Additionally, a new function for unsetting query responses has been introduced, along with corresponding tests. The testing framework has also been refined with a factory-based approach.

Changes

Files Change Summary
precompile/modules/initia_stdlib/sources/dex.move
precompile/modules/minitia_stdlib/sources/dex.move
Ratio calculation updated to use decimal256::from_ratio for improved accuracy; numerator and denominator calculations refined.
precompile/modules/initia_stdlib/sources/token/collection.move
precompile/modules/minitia_stdlib/sources/token/collection.move
Assertion for collection name format changed from checking for "::" to ":", impacting validation logic.
precompile/modules/initia_stdlib/sources/token/nft.move
precompile/modules/minitia_stdlib/sources/token/nft.move
Assertion for token ID format changed from checking for "::" to ":", allowing a broader range of valid token IDs.
precompile/modules/initia_stdlib/sources/query.move
precompile/modules/minitia_stdlib/sources/query.move
New function unset_query_response added for clearing query responses, along with a test for validation of this functionality.
Cargo.toml Updated version control references for multiple dependencies to a new commit, indicating a shift to potentially more stable or feature-rich versions.
crates/compiler/src/lib.rs Renamed module from test_gas_meter to unit_test_factory, altering the organizational structure without introducing new logic.
crates/compiler/src/test_package.rs Replaced gas meter testing mechanism with a factory-based approach, enhancing test flexibility and configurability.
crates/compiler/src/unit_test_factory.rs Introduced InitiaUnitTestFactory struct for unit testing with gas metering capabilities, enhancing the testing framework.
precompile/modules/initia_stdlib/sources/json.move
precompile/modules/minitia_stdlib/sources/json.move
Minor formatting adjustments in test functions for consistent spacing, ensuring code readability without affecting logic.

Poem

In fields where bunnies hop and play,
Ratios are bright, in a brand-new way!
With single colons leading the cheer,
Valid names bloom, bringing joy near.
Hops of delight with code so fine,
Celebrate changes, it's our time to shine! 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9afe29f and 0ddb4cf.

Files selected for processing (6)
  • precompile/modules/initia_stdlib/sources/dex.move (1 hunks)
  • precompile/modules/initia_stdlib/sources/token/collection.move (1 hunks)
  • precompile/modules/initia_stdlib/sources/token/nft.move (1 hunks)
  • precompile/modules/minitia_stdlib/sources/dex.move (1 hunks)
  • precompile/modules/minitia_stdlib/sources/token/collection.move (1 hunks)
  • precompile/modules/minitia_stdlib/sources/token/nft.move (1 hunks)
Additional comments not posted (6)
precompile/modules/initia_stdlib/sources/token/nft.move (1)

91-93: Adjust token ID validation logic.

The assert_token_id function now checks for a single colon ":" instead of a double colon "::". This change broadens the range of valid token IDs. Ensure that this aligns with the intended business logic and does not introduce security vulnerabilities.

precompile/modules/minitia_stdlib/sources/token/nft.move (1)

91-93: Adjust token ID validation logic.

The assert_token_id function now checks for a single colon ":" instead of a double colon "::". Ensure this change is consistent with the intended validation requirements and does not introduce security issues.

precompile/modules/initia_stdlib/sources/token/collection.move (1)

213-215: Adjust collection name validation logic.

The assert_collection_name function now checks for a single colon ":" instead of a double colon "::". Ensure this change aligns with the intended validation requirements and does not introduce inconsistencies.

Verification successful

Collection name validation logic is consistent.

The assert_collection_name function correctly checks for a single colon at the end of the collection name. This change aligns with the intended validation requirements and is consistently applied in the codebase.

  • The function ensures the collection name ends with a single colon ":".
  • This logic is consistently used where assert_collection_name is called.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `assert_collection_name` to ensure they align with the new validation logic.

# Test: Search for the function usage. Expect: Only occurrences of the new logic.
rg --type move -A 5 $'assert_collection_name'

Length of output: 73


Script:

#!/bin/bash
# Description: Search for the function `assert_collection_name` usage in `.move` files to verify the new validation logic.

# Find all `.move` files and search for `assert_collection_name` usage
fd --extension move --exec rg 'assert_collection_name' {}

Length of output: 233


Script:

#!/bin/bash
# Description: Search for the function `assert_collection_name` usage in `.move` files with additional context to verify the new validation logic.

# Find all `.move` files and search for `assert_collection_name` usage with context
fd --extension move --exec rg 'assert_collection_name' -A 10 -B 2 {}

Length of output: 1612

precompile/modules/minitia_stdlib/sources/token/collection.move (1)

213-215: Adjust collection name validation logic.

The assert_collection_name function now checks for a single colon ":" instead of a double colon "::". Ensure this change aligns with the intended validation requirements and does not introduce inconsistencies.

precompile/modules/initia_stdlib/sources/dex.move (1)

264-269: Enhance ratio calculation accuracy.

The get_spot_price function now uses division instead of multiplication for calculating the numerator and denominator, improving accuracy. Ensure this change is consistent with the intended mathematical logic and does not introduce any unintended side effects.

precompile/modules/minitia_stdlib/sources/dex.move (1)

264-269: Verify the correctness of division logic for ratio calculation.

The change from multiplication to division in calculating the numerator and denominator could significantly impact the results. Ensure that this logic aligns with the intended mathematical model and does not introduce errors in scenarios where weights and pools vary.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0ddb4cf and a1fe145.

Files selected for processing (2)
  • precompile/modules/initia_stdlib/sources/coin.move (2 hunks)
  • precompile/modules/minitia_stdlib/sources/coin.move (2 hunks)
Files skipped from review due to trivial changes (1)
  • precompile/modules/initia_stdlib/sources/coin.move
Additional comments not posted (2)
precompile/modules/minitia_stdlib/sources/coin.move (2)

Line range hint 378-391: Renaming enhances clarity.

The renaming of initialized_coin to initialize_coin_for_testing improves clarity by explicitly indicating the function's purpose in testing contexts. The logic and parameters remain unchanged, maintaining functionality.


399-400: Function call updated for consistency.

The function call to initialize_coin_for_testing reflects the updated function name, maintaining consistency and clarity in the test logic.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a1fe145 and fc0cdd9.

Files selected for processing (3)
  • crates/natives/src/query.rs (2 hunks)
  • precompile/modules/initia_stdlib/sources/query.move (2 hunks)
  • precompile/modules/minitia_stdlib/sources/query.move (2 hunks)
Additional comments not posted (5)
precompile/modules/initia_stdlib/sources/query.move (2)

82-85: Ensure the unset_query_response function is necessary.

The unset_query_response function is marked with #[test_only], indicating it's intended for testing purposes. Ensure that this functionality is essential for the testing strategy and doesn't introduce unnecessary complexity.


103-111: Clarify the expected failure in test_query_unsset.

The test is annotated with #[expected_failure], which suggests it is expected to fail under certain conditions. Ensure that the abort code 0x1006E and the failure conditions are well-documented and understood.

precompile/modules/minitia_stdlib/sources/query.move (2)

83-86: Ensure the unset_query_response function is necessary.

The unset_query_response function is marked with #[test_only], indicating it's intended for testing purposes. Ensure that this functionality is essential for the testing strategy and doesn't introduce unnecessary complexity.


104-111: Clarify the expected failure in test_query_unsset.

The test is annotated with #[expected_failure], which suggests it is expected to fail under certain conditions. Ensure that the abort code 0x1006E and the failure conditions are well-documented and understood.

crates/natives/src/query.rs (1)

346-371: Verify the logic in native_test_only_unset_query_response.

The function removes a query response based on a computed hash. Ensure that the hash computation and removal logic are correct and consistent with the intended functionality.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between fc0cdd9 and 7c7b064.

Files selected for processing (6)
  • precompile/modules/initia_stdlib/sources/dex.move (2 hunks)
  • precompile/modules/initia_stdlib/sources/json.move (1 hunks)
  • precompile/modules/initia_stdlib/sources/query.move (2 hunks)
  • precompile/modules/minitia_stdlib/sources/dex.move (2 hunks)
  • precompile/modules/minitia_stdlib/sources/json.move (1 hunks)
  • precompile/modules/minitia_stdlib/sources/query.move (2 hunks)
Files skipped from review due to trivial changes (2)
  • precompile/modules/initia_stdlib/sources/json.move
  • precompile/modules/minitia_stdlib/sources/json.move
Files skipped from review as they are similar to previous changes (2)
  • precompile/modules/initia_stdlib/sources/query.move
  • precompile/modules/minitia_stdlib/sources/query.move
Additional comments not posted (2)
precompile/modules/initia_stdlib/sources/dex.move (1)

265-271: Verify conversion from decimal256 to decimal128.

The use of decimal256 enhances precision, but ensure that the conversion back to decimal128 does not lead to precision loss or overflow.

Run the following script to verify the usage of decimal128::new and check for potential precision issues:

precompile/modules/minitia_stdlib/sources/dex.move (1)

265-271: Verify conversion from decimal256 to decimal128.

The use of decimal256 enhances precision, but ensure that the conversion back to decimal128 does not lead to precision loss or overflow.

Run the following script to verify the usage of decimal128::new and check for potential precision issues:

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7c7b064 and 5ae5923.

Files selected for processing (4)
  • Cargo.toml (1 hunks)
  • crates/compiler/src/lib.rs (1 hunks)
  • crates/compiler/src/test_package.rs (3 hunks)
  • crates/compiler/src/unit_test_factory.rs (1 hunks)
Files skipped from review due to trivial changes (1)
  • crates/compiler/src/lib.rs
Additional comments not posted (7)
crates/compiler/src/unit_test_factory.rs (4)

9-12: Struct definition looks good.

The InitiaUnitTestFactory struct is well-defined with appropriate fields.


15-20: Constructor implementation is correct.

The new function correctly initializes InitiaUnitTestFactory with the provided parameters.


22-39: Gas charging logic is well-implemented.

The charge_write_set_gas function effectively charges gas for the write set and handles errors properly.


43-71: UnitTestFactory implementation is solid.

The implementation of UnitTestFactory for InitiaUnitTestFactory is correct, with proper session handling and gas usage calculation.

crates/compiler/src/test_package.rs (2)

11-18: Imports are correctly updated.

The import statements are updated to include the necessary modules for the new factory-based approach.


Line range hint 49-69: Function changes align with the factory-based approach.

The execute function now uses InitiaUnitTestFactory and run_move_unit_tests_with_factory, enhancing test flexibility.

Cargo.toml (1)

115-135: Dependency updates are consistent.

The revision identifiers for the dependencies have been updated consistently, indicating a shift to a potentially more stable or feature-rich version.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5ae5923 and 796a7cc.

Files selected for processing (2)
  • Cargo.toml (1 hunks)
  • crates/compiler/src/unit_test_factory.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • Cargo.toml
  • crates/compiler/src/unit_test_factory.rs

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 796a7cc and 831ad6c.

Files ignored due to path filters (2)
  • api/libcompiler.dylib is excluded by !**/*.dylib
  • api/libmovevm.dylib is excluded by !**/*.dylib
Files selected for processing (4)
  • Cargo.toml (1 hunks)
  • crates/compiler/src/test_package.rs (3 hunks)
  • crates/compiler/src/unit_test_factory.rs (1 hunks)
  • crates/e2e-move-tests/src/tests/move_unit.rs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
  • Cargo.toml
  • crates/compiler/src/unit_test_factory.rs
Additional comments not posted (11)
crates/compiler/src/test_package.rs (4)

11-11: Updated import for factory-based unit tests.

The import statement has been updated to use run_move_unit_tests_with_factory, reflecting the shift to a factory-based approach. This change aligns with the new testing strategy.


18-18: Updated import for InitiaUnitTestFactory.

The import statement now includes InitiaUnitTestFactory, indicating the use of a factory pattern for unit tests. This change enhances test flexibility.


49-49: Instantiation of InitiaUnitTestFactory.

The InitiaUnitTestFactory is instantiated with gas parameters and a gas limit, replacing the previous gas meter instantiation. This change supports the new factory-based testing approach.


Line range hint 53-69: Use of run_move_unit_tests_with_factory.

The function run_move_unit_tests_with_factory is used to execute unit tests, replacing the previous method. This change leverages the factory pattern for improved test execution.

crates/e2e-move-tests/src/tests/move_unit.rs (7)

7-7: Updated import for gas parameters.

The import statement now includes InitiaGasParameters and InitialGasSchedule, indicating a shift to initializing gas parameters with potentially more meaningful values.


15-15: Import for InitiaUnitTestFactory.

The import statement includes InitiaUnitTestFactory, reflecting the use of a factory pattern for unit tests. This change supports the new testing strategy.


17-17: Updated import for factory-based unit tests.

The import statement has been updated to use run_move_unit_tests_with_factory, aligning with the new factory-based testing approach.


55-56: Initialization of gas parameters.

Gas parameters are now initialized with InitiaGasParameters::initial(), replacing the previous zero initialization. This change ensures that gas costs are appropriately accounted for.


56-56: Instantiation of InitiaUnitTestFactory.

The InitiaUnitTestFactory is instantiated with gas parameters and a gas limit, supporting the new factory-based testing approach.


58-59: Initialization of native and misc gas parameters.

Native and misc gas parameters are now initialized with initial() values, replacing the previous zero initialization. This change ensures meaningful gas parameter values.


71-79: Use of run_move_unit_tests_with_factory.

The function run_move_unit_tests_with_factory is used to execute unit tests, reflecting the transition to a factory-based approach. This change enhances test configurability.

@beer-1 beer-1 merged commit a888a5d into main Aug 23, 2024
5 checks passed
@beer-1 beer-1 deleted the fix/audit branch August 23, 2024 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant