[Cost updated] Improve accuracy of cost calculations #219
Conversation
…ance cost updated flow to preserve precision; add tests
00_Base/test/money/Money.test.ts
Outdated
There was a problem hiding this comment.
I have added some tests for base operations on Money, if you have any suggestions for additional test cases or corner cases we should address, please let me know and I will incorporate them!
There was a problem hiding this comment.
very cool! just curious, was there a reason to not use a library such as currency.js (also understandable to not use bc we don't want more dependencies 😬)
@yuvenus good point! I looked into using a library for handling monetary values and found two potentially useful options: DineroJS and currency.js.
After researching, many recommend using libraries like Big.js or Decimal.js for money handling. Implementing our own Money type based on Big.js could be beneficial, as it allows us to design the API to suit our needs. I’ve written some tests, and it seems to work well so far. This approach aligns with practices in other languages, like Java’s Moneta, which is based on BigDecimal (though some prefer using longs). What are your thoughts? Should we consider DineroJS v2 alpha, or stick to implementing our own Money based on Big.js? Additionally, it might be worth considering a reconciliation process to periodically check for discrepancies over longer periods, such as daily or weekly, what do you think? |
regarding the libraries, makes sense! better safe than sorry and implement exactly as we need. it blows my mind a little bit though that no one's really solved all the problems needed to handle currency in javascript, but it is what it is. (perhaps it's time to release regarding the reconciliation process, what discrepancies do you anticipate? is it things like the decimal precision being wrong, currencies changing, currency formats changing, etc...? i agree in general if we're maintaining essentially our own currency "library", there should be ways to make sure our currency processing isn't going out of date - but i'm not sure what this would look like in the long term 🤔 |
I was surprised too that I couldn’t find a suitable library - each one had its drawbacks. DineroJS v2 looks promising but seems a bit inactive and has been in alpha for several years 🫤 I like the name Regarding reconciliation, maybe that wasn’t the best term; what I meant is that we could compare the total of all costs (that are each rounded down) over a period of time (e.g. a day) with a single large cost where we would round only once at the end after summing up all costs. |
| switch (typeof predicate) { | ||
| case 'boolean': { | ||
| if (!predicate) { | ||
| throw new Error(message); |
There was a problem hiding this comment.
What do you think about adding a custom AssertionError class for this?
* rc-1.4.0: (106 commits) chore: resolving build error fix: run only install for jest tests to avoid workspaces as they aren't needed fix: add run build for finding lower packages. fix: also make sure parent level dependancies are installed. fix: run install all with workspace and bump npm cache version to get away from EOL node version alert feat: add github action to run npm test refactor: Standardize naming convention with suffix `Provider` and capitalcase filenames for tests fix: resolving this.readNextId undefined error chore: ensuring node_modules is ignored chore: removing ajv from swarm and setting new version in base chore: fixing ajv conflicts chore: updating eslint / tslint setup to newer version and removing no longer supported files but instead using the new eslint.config.js following all recommended settings overriding only the no-explicit-any and no-unused-vars values and setting the ignores ocpi-reservation: changes based on feedback chore: reversting linting changes chore: adjusting all dependency versions to be static where possible, in a single case, "@fastify/swagger-ui": "1.9.3", enforces us to use "@fastify/swagger": "^8.0.0". Making versions static because in OCPI the latest renovate changes that made all of the dependencies static now introduce a lot of conflicts in any overlapping dependencies that in core still contain the carrot ^ fix: resoling issue with Ajv version conflicts as it is used in a few other libraries and to make sure that we are using it as intended by moving to base and exporting from there to be imported into other modules from base instead of ajv directly ensuring the correct version is applied. chore: fixing fastify swagger.ts build error, also seemingly version related, by wrapping in "as any" which works chore: aligning @typescript-eslint/eslint-plugin version Revert "chore: simplifying lint commands" chore: simplifying lint commands fix: issue with Ajv conflicting with the version that fastify is bringing by overriding in package.json and ensuring that we are importing it correctly to resolve type error chore: removing --workspaces to ensure that all dev dependencies are installed along with the workspace dependencies otherwise some dependencies are missed. ... # Conflicts: # 00_Base/package.json # 00_Base/src/index.ts
* rc-1.4.0: (24 commits) enhancing boot notification service tests: removing unnecessary added awaits clear variable monitoring tests: fixing import in unit test to use the correct path clear variable monitoring tests: simplifying imports in Monitoring module clear variable monitoring tests: adding jest config to package json test script + adding unit tests for processClearMonitoringResult clear variable monitoring tests: adding base for MonitoringService test clear variable monitoring tests: adding MonitoringService and moving clear variable monitoring handler logic into service enhancing boot notification service tests: removing maxCachingSeconds as a constructor argument from remaining files + adding more comments enhancing boot notification service tests: removing maxCachingSeconds as a constructor argument for BootNotificationService and instead passing it in to the relevant methods that need it + moving more cache-related calls back into the module since they're not relevant to the methods they were put into enhancing boot notification service tests: removing unused imports from DeviceModelService enhancing boot notification service tests: reverting pulling out of set variables since it's not as easy to do as initially thought enhancing boot notification service tests: undoing some condition simplifications enhancing boot notification service tests: rename from executeSetVariablesActionsAfterBoot to executeSetVariablesAfterBoot for brevity enhancing boot notification service tests: fixing name of helper variable from bootWithRejectedVariables to doNotBootWithRejectedVariables for clarity enhancing boot notification service tests: updating condition to trigger set variables if pendingBootSetVariables.length > 0, rather than > 1 enhancing boot notification service tests: fixing build issues with BootNotificationService test enhancing boot notification service tests: removing unneeded commented code in DeviceModelService enhancing boot notification service tests: adding tests for BootNotificationService's "triggerGetBaseReport" method enhancing boot notification service tests: adding tests for BootNotificationService's "cacheChargerActionsPermissions" method enhancing boot notification service tests: refactoring more functionality out of Configuration module's BootNotification handler to BootNotificationService + DeviceModelService to prepare for unit tests enhancing boot notification service tests: updating boot servivce constructor to take in a cache + logger to support some of the boot notification handler functionality that was moved into the service ...
Previously, cost calculations were performed using standard JavaScript numbers, which introduced floating-point issues and resulted in inaccurate results.
This PR introduces a Money and a Currency classes to better handle monetary values. The Money class is implemented using the Big.js library to ensure accuracy. All monetary operations are delegated to this class to avoid precision loss.