-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feat/table asset main #716
Conversation
WalkthroughThis update introduces a new feature for updating asset information in the system. It includes a configuration for a periodic job, a migration script for creating an asset database table, and the necessary model and service to handle asset updates. Additionally, constants are updated to support this new feature, and unit tests are added to ensure its correct functionality. Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
config.json
is excluded by:!**/*.json
Files selected for processing (6)
- ci/config.json.ci (1 hunks)
- migrations/20240110065150_create_asset_model.ts (1 hunks)
- src/common/constant.ts (2 hunks)
- src/models/asset.ts (1 hunks)
- src/services/job/update_assets.service.ts (1 hunks)
- test/unit/services/job/update_assets.spec.ts (1 hunks)
Additional comments: 12
migrations/20240110065150_create_asset_model.ts (3)
- 10-10: Consider changing the
price
field type fromfloat
todecimal
to avoid precision issues, especially since this field likely represents financial data. Usingdecimal
provides exact precision, which is crucial for financial calculations.- 11-11: The
total_supply
field definition usingdecimal(80, 0)
is appropriate for representing large numbers without fractional parts, suitable for asset supply quantities.- 13-13: The
updated_at
field is correctly defined with atimestamp
type, set to not nullable, and defaults to the current timestamp. This ensures that every record will have an accurate timestamp upon creation.src/models/asset.ts (2)
- 5-24: The property definitions in the
Asset
class are well-defined and consistent with the database schema. The use of TypeScript's non-null assertion operator is appropriate, assuming these fields will always be populated. The inclusion ofibc_channel
indicates a well-thought-out relational data model.- 30-40: The static properties
TYPE
andPREFIX
in theAsset
class are well-defined and promote consistency throughout the application by avoiding hard-coded strings. This is a good practice for maintainability and readability.ci/config.json.ci (1)
- 298-300: The addition of the
jobUpdateAssets
configuration withmillisecondRepeatJob
andlcdRecordGet
parameters is well-defined. Ensure that the values for these parameters (10000
formillisecondRepeatJob
and5
forlcdRecordGet
) are chosen based on the application's specific needs and performance considerations.test/unit/services/job/update_assets.spec.ts (3)
- 81-95: The setup and teardown methods using
BeforeAll
,AfterAll
, andAfterEach
decorators are correctly implemented to ensure a clean and isolated test environment. The use ofTRUNCATE TABLE
in the setup is particularly good practice for maintaining test isolation.- 103-103: The test case lacks a descriptive name. Consider adding a meaningful name that clearly describes what the test case is verifying. This improves readability and maintainability of the test suite.
- 103-216: Consider enhancing the assertions in the test case to verify more properties of the assets, ensuring a thorough test coverage. This could include checking for specific values of properties like
type
,decimal
,name
, and others, to ensure theUpdateAssetsJob
service is functioning as expected.src/services/job/update_assets.service.ts (1)
- 31-237: The implementation of the
UpdateAssetsJob
service, including querying assets from RPC, handling origin assets, and updating assets in the database, is well-structured and follows best practices. Ensure that floating-point numbers, such astotal_supply
, are handled carefully to avoid precision issues. The use ofonConflict('denom').merge()
for upserting assets is particularly noteworthy.src/common/constant.ts (2)
- 124-124: The addition of the
JOB_UPDATE_ASSETS
constant follows the established naming conventions and seems appropriate for the described functionality in the PR objectives.- 349-352: The addition of the
UpdateAssets
entry to theSERVICE
object is consistent with the structure and conventions used throughout the file. It's clear and well-defined, facilitating easy identification and access to the service.
Summary by CodeRabbit
New Features
Tests
Chores