Increasing trades.price decimal scale #89
Conversation
…structive rounding when calculating transaction cost; changes to the UI to allow for inputting and showing increased scale trade prices; test case
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mrbaboo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
The primary goal of this pull request is to address an issue where trades.price was being destructively rounded due to an insufficient decimal scale in the database and UI, leading to inaccurate transaction cost calculations. I've implemented changes to increase the precision of trade prices, ensuring that highly granular transaction costs can be accurately tracked and displayed.
Highlights
- Database Precision Increase: I've introduced a new migration to expand the decimal scale of the
trades.pricecolumn in the database from 4 to 10, allowing for much finer granularity in price storage. - UI Field Customization: I've updated the
_money_fieldpartial to acceptprecisionandstepoptions, making it more flexible for different numerical input requirements. - Trade Price UI Updates: I've adjusted the
money_fieldcalls forTrade.pricein both the trade creation form and the trade show page to utilize the newprecision: 10andstep: 'any'options, aligning the UI with the increased database precision. - Precision Test Coverage: I've added new unit tests for the
Trademodel to explicitly verify that prices are stored and retrieved with the intended 10 decimal places of precision, and that rounding occurs correctly beyond this limit.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request successfully increases the decimal precision for trade prices to avoid rounding issues. The changes are consistently applied to the database, views, and are accompanied by new tests. I have a couple of suggestions: one to fix a new test that isn't behaving as intended, and another to improve the robustness of the _money_field partial.
|
Wow, super-cool "improve the plumbing" contribution! Do you mind if we slot this for a |
|
(put this in "draft" mode if you agree) |
|
No problem @jjmata - it's now in draft mode. That will give me some time to address the Gemini and lint problems, too. Do I need to change it back to open once 0.7.1 is released or will you do that? |
|
So the work on #94 for SimpleFIN means we'll be changing schema after all ... so maybe we should target to include this as well in |
Signed-off-by: Juan José Mata <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces dynamic precision and step handling in the shared money field. Updates trade price inputs to use step: "any" and precision: 10. Adds a migration increasing trades.price scale from 4 to 10, updates schema, and adds tests verifying 10-decimal storage and rounding behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| end | ||
|
|
||
| def down | ||
| change_column :trades, :price, :decimal, precision: 19, scale: 4 |
There was a problem hiding this comment.
Rolling back the migration can fail or cause data loss because reducing the scale from 10 to 4 is not safely reversible.
Prompt for AI agents
Address the following comment on db/migrate/20250806155348_increase_trade_price_precision.rb at line 7:
<comment>Rolling back the migration can fail or cause data loss because reducing the scale from 10 to 4 is not safely reversible.</comment>
<file context>
@@ -0,0 +1,9 @@
+class IncreaseTradePricePrecision < ActiveRecord::Migration[7.2]
+ def up
+ change_column :trades, :price, :decimal, precision: 19, scale: 10
+ end
+
+ def down
+ change_column :trades, :price, :decimal, precision: 19, scale: 4
+ end
+end
</file context>
![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
🧹 Nitpick comments (4)
db/migrate/20250806155348_increase_trade_price_precision.rb (1)
2-8: Migration correct but watch out for locking on large tablesThe
upanddownmethods correctly adjust thepricecolumn’s precision and scale. However, changing a column’s type on a largetradestable can acquire an exclusive write lock and impact availability.• Since I don’t see the strong_migrations gem in your Gemfile or codebase, you won’t be able to use
safety_assuredout of the box.
• You should either:
- Schedule this migration during a maintenance window to minimize impact, or
- Implement a zero-downtime migration strategy (e.g., adding a new column, backfilling, and swapping columns).
If you decide to adopt the strong_migrations gem in the future, you can wrap this in
safety_assured { change_column … }to bypass its locking checks.test/models/trade_test.rb (1)
41-56: Rounding assertion is sound; consider adding a boundary case and fixtures.
- Optional: add a boundary test near the integer limit (e.g., 999_999_999.12345678901) to ensure DB+AR round to 10 decimals without overflow.
- Per repo testing guidelines, prefer fixtures for simple associated records (Security) over ad-hoc creation.
If helpful, I can add a small fixture and an extra boundary test in a follow-up commit.
app/views/shared/_money_field.html.erb (1)
51-55: Avoid Float conversion in formatting to preserve BigDecimal fidelity.
sprintf("%f", val)coercesBigDecimaltoFloat, which can introduce rounding artifacts for values with many significant digits (e.g., 9 integer + 10 fractional). Use Rails’number_with_precisionto format without losing precision and to keep trailing zeros.Apply this diff:
- sprintf("%.#{options[:precision].presence || currency.default_precision}f", options[:value]) + number_with_precision( + options[:value], + precision: (options[:precision].presence || currency.default_precision), + strip_insignificant_zeros: false, + separator: ".", + delimiter: "" + ) @@ - sprintf("%.#{options[:precision].presence || currency.default_precision}f", val) if val.present? + number_with_precision( + val, + precision: (options[:precision].presence || currency.default_precision), + strip_insignificant_zeros: false, + separator: ".", + delimiter: "" + ) if val.present?Note: specifying
separator: "."anddelimiter: ""ensures we emit an HTML5 number-compatible value regardless of locale.app/views/trades/_form.html.erb (1)
53-53: Optional: align HTML step to the 10-decimal scale for better spinner behaviorUsing step: 'any' disables step-mismatch validation, which is fine, but number inputs will typically step by 1 with arrow keys. If you want the spinner to reflect the 10-decimal precision, consider a precise step value.
Apply this minimal diff:
-<%= form.money_field :price, label: t(".price"), step: 'any', precision: 10, required: true %> +<%= form.money_field :price, label: t(".price"), step: '0.0000000001', precision: 10, required: true %>Note: Keep 'any' if you explicitly prefer disabling browser step validation and avoiding step rounding edge cases; this is a UX trade-off, not a correctness issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/views/shared/_money_field.html.erb(1 hunks)app/views/trades/_form.html.erb(1 hunks)app/views/trades/show.html.erb(1 hunks)db/migrate/20250806155348_increase_trade_price_precision.rb(1 hunks)db/schema.rb(1 hunks)test/models/trade_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
{app,lib}/**/*.{rb,erb}
📄 CodeRabbit inference engine (CLAUDE.md)
{app,lib}/**/*.{rb,erb}: Use Current.user for the authenticated user; do not use current_user
Use Current.family for the current family; do not use current_family
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (CLAUDE.md)
{app/views,app/components}/**/*.html.erb: In views, always use the icon helper; never call lucide_icon directly
Prefer semantic HTML and server-rendered Hotwire patterns (Turbo) over client-side JS widgets
Use Tailwind functional tokens (e.g., text-primary, bg-container, border-primary) instead of raw color utilities like text-white or bg-white
Keep domain/business logic out of view templates; views should primarily render data
Stimulus controllers must use declarative actions (data-action and data--target) rather than inline JavaScript
Pass data to Stimulus via data--value attributes; do not embed inline JavaScript
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/**/*.rb,app/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
{app/**/*.rb,app/**/*.erb}: UseCurrent.userfor the current user; do not usecurrent_userin Rails code and templates
UseCurrent.familyfor the current family; do not usecurrent_familyin Rails code and templates
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native semantic HTML elements over custom JS components (e.g., for modals,/for disclosures)
Use Turbo Frames to segment pages instead of JS-driven client-side solutions where possible
Prefer client-side form validation when possible for UX
app/views/**/*.html.erb: Use ViewComponents when the element has complex logic/styling, is reused across contexts, needs variants/sizes or slots, requires interactivity/Stimulus, or needs accessibility/ARIA support
Use partials when the element is primarily static HTML with minimal logic, used in few contexts, simple template content, has no variants/configuration, and is mainly for content organization
Prefer components over partials: if a suitable component exists in app/components, use it; otherwise consider a partial or create a component based on the criteria
Integrate Stimulus declaratively in views: HTML declares actions/targets; controllers respond (avoid imperative JS in templates)
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/helpers/**/*.rb,app/models/**/*.rb,app/views/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates server-side (helpers/models) and pass formatted values to Stimulus for display only
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/views/**/*.{html,html+turbo-stream}.erb,app/helpers/**/*.rb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Always use the icon helper (icon) from application_helper.rb; never call lucide_icon directly
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/javascript/controllers/**/*.@(js|ts),app/components/**/*.@(js|ts),app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controllers (e.g., avoid addEventListener in connect/initializers).
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/components/**/*.@(js|ts),app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component Stimulus controllers under app/components must only be used within their component views, not in app/views.
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Author styles assuming TailwindCSS v4.x is in use across the app
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand the base primitives, functional tokens, and component tokens
Prefer functional tokens from maybe-design-system.css over raw colors/utilities (e.g., usetext-primaryovertext-white,bg-containeroverbg-white,border border-primaryoverborder border-gray-200)
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
{app/views/**/*.html.erb,app/components/**/*.html.erb}
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views/**/*.html.erb,app/components/**/*.html.erb}: Keep domain logic out of views; compute classes and logic in component/view helpers, not inside ERB templates
Pass data from Rails to Stimulus via data-*-value attributes; do not embed inline JavaScript in templates
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erbapp/views/trades/show.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with an underscore prefix (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erb
app/views/shared/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Place shared partials under app/views/shared/
Files:
app/views/shared/_money_field.html.erb
test/**/*_test.rb
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*_test.rb: Use VCR for external API testing
Use the mocha gem for stubs and mocks in tests
Files:
test/models/trade_test.rb
test/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
test/**/*.rb: Write tests with Minitest; do not use RSpec in this codebase
Use fixtures for test data; do not use factories
For tests needing many records, use Rails helpers to create them and inline the creation in the test (e.g., helpers in test/support)
Test boundaries correctly: for query methods, assert returned output; for command methods, assert the command was invoked with correct params
Do not test one class’s implementation details in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for simple mock instances, or define a small mock class for complex cases
Only mock what’s necessary; don’t mock return values you don’t assert on
Files:
test/models/trade_test.rb
app/views/!(shared)/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Place context-specific partials in the relevant controller's view directory (not in shared/)
Files:
app/views/trades/_form.html.erb
db/migrate/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Enforce simple validations (NOT NULL, unique indexes) at the database level
Files:
db/migrate/20250806155348_increase_trade_price_precision.rb
🧠 Learnings (2)
📚 Learning: 2025-08-12T01:02:31.793Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-08-12T01:02:31.793Z
Learning: Applies to {app/helpers/**/*.rb,app/models/**/*.rb,app/views/**/*.html.erb} : Format currencies, numbers, and dates server-side (helpers/models) and pass formatted values to Stimulus for display only
Applied to files:
app/views/shared/_money_field.html.erbapp/views/trades/_form.html.erb
📚 Learning: 2025-08-12T01:02:31.793Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-08-12T01:02:31.793Z
Learning: Applies to app/javascript/**/*.js : Avoid implementing currency/number/date formatting in client-side Stimulus/JS; keep controllers for display behavior
Applied to files:
app/views/shared/_money_field.html.erb
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ci / test
🔇 Additional comments (5)
db/schema.rb (1)
792-792: Verification needed: script failed in sandbox due to missing Ruby runtimeThe provided shell check didn’t execute (error:
/usr/bin/env: 'ruby': No such file or directory), so we couldn’t confirm whether any existing trades exceed 9 integer digits. Please run one of the following in your local app environment to verify:• With Rails runner:
bin/rails runner 'puts({ max: Trade.maximum(:price), over: Trade.where("ABS(price) > 999_999_999").count }.inspect)'• Direct SQL via
rails dbconsoleor your DB client:SELECT MAX(price) AS max_price FROM trades; SELECT COUNT(*) AS over_limit FROM trades WHERE ABS(price) > 999999999;Once you’ve confirmed that no records exceed the new 9-digit integer limit (or added appropriate validations/import guards), this change is safe to merge.
test/models/trade_test.rb (1)
24-39: Test correctly asserts exact persistence at 10 decimals.Good coverage making sure a value with exactly 10 decimals round-trips unchanged.
app/views/trades/show.html.erb (1)
43-45: UI aligned with DB precision; good call onstep: "any".Passing
precision: 10andstep: "any"avoids browser-side coercion and matches the DB scale. Looks good.app/views/shared/_money_field.html.erb (1)
59-59: Step override looks good and remains backward compatible.
options[:step] || currency.stepkeeps prior behavior while allowing call sites to opt into higher precision. No issues.app/views/trades/_form.html.erb (1)
53-53: LGTM: enable 10-decimal precision and free-form input for priceThis change matches the DB scale increase and the updated money_field options. It prevents destructive rounding in the UI while preserving server-side formatting responsibilities.
Want to review the AI-bot comments here, @mrbaboo? We should be able to take this into |
commit 192a3b6 Author: soky srm <[email protected]> Date: Wed Oct 22 17:03:00 2025 +0200 Implement a filter for category (we-promise#215) - Also implement an is empty/is null condition. commit 8cd109a Author: soky srm <[email protected]> Date: Wed Oct 22 16:02:50 2025 +0200 Implement support for generic OpenAI api (we-promise#213) * Implement support for generic OpenAI api - Implements support to route requests to any openAI capable provider ( Deepsek, Qwen, VLLM, LM Studio, Ollama ). - Keeps support for pure OpenAI and uses the new better responses api - Uses the /chat/completions api for the generic providers - If uri_base is not set, uses default implementation. * Fix json handling and indentation * Fix linter error indent * Fix tests to set env vars * Fix updating settings * Change to prefix checking for OAI models * FIX check model if custom uri is set * Change chat to sync calls Some local models don't support streaming. Revert to sync calls for generic OAI api * Fix tests * Fix tests * Fix for gpt5 message extraction - Finds the message output by filtering for "type" == "message" instead of assuming it's at index 0 - Safely extracts the text using safe navigation operators (&.) - Raises a clear error if no message content is found - Parses the JSON as before * Add more langfuse logging - Add Langfuse to auto categorizer and merchant detector - Fix monitoring on streaming chat responses - Add Langfuse traces also for model errors now * Update app/models/provider/openai.rb Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: soky srm <[email protected]> * handle nil function results explicitly * Exposing some config vars. * Linter and nitpick comments * Drop back to `gpt-4.1` as default for now * Linter * Fix for strict tool schema in Gemini - This fixes tool calling in Gemini OpenAI api - Fix for getTransactions function, page size is not used. --------- Signed-off-by: soky srm <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Juan José Mata <[email protected]> commit ea7ce13 Author: Michael Studman <[email protected]> Date: Wed Oct 22 03:22:24 2025 +1100 Increasing trades.price decimal scale (we-promise#89) * Changing trades.price to have a larger scale - a scale of 4 causes destructive rounding when calculating transaction cost; changes to the UI to allow for inputting and showing increased scale trade prices; test case commit 3aea151 Author: Pedro Camara Junior <[email protected]> Date: Tue Oct 21 15:34:44 2025 +0200 Add comprehensive Hetzner Cloud deployment guide (we-promise#211) * Add comprehensive Hetzner Cloud deployment guide * Fix markdown linting issues and backup retention policy - Add missing language identifiers to fenced code blocks (bash) - Fix inconsistent backup retention policy (standardize to 7 days) - Address CodeRabbit review feedback for PR we-promise#211 commit eaa17fe Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue Oct 21 15:02:57 2025 +0200 Bump rack from 3.1.16 to 3.1.18 (we-promise#198) Bumps [rack](https://github.com/rack/rack) from 3.1.16 to 3.1.18. - [Release notes](https://github.com/rack/rack/releases) - [Changelog](https://github.com/rack/rack/blob/main/CHANGELOG.md) - [Commits](rack/rack@v3.1.16...v3.1.18) --- updated-dependencies: - dependency-name: rack dependency-version: 3.1.18 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Changing trades.price to have a larger scale - a scale of 4 causes destructive rounding when calculating transaction cost; changes to the UI to allow for inputting and showing increased scale trade prices; test case
* Changing trades.price to have a larger scale - a scale of 4 causes destructive rounding when calculating transaction cost; changes to the UI to allow for inputting and showing increased scale trade prices; test case
2 participants
A scale of 4 causes for trades.price causes destructive rounding when calculating transaction cost. The UI was imposing a scale of 2, in any case.
e.g. 20000 shares bought for a total of 101,777.88 USD is 5.088894 USD per share (average cost). The UI forces that to be rounded to 5.08. The DB would only store 5.0888 anyway. So this causes the total transaction cost to be represented as 101,600 USD. There just isn't a way to track actual transaction cost accurately for some transactions.
This change:
Summary by CodeRabbit