Fees should use the right units
Follow-up to !5 (comment 40410)
Looking at the code in develop right now I think it still suffers from one of the issues I mentioned, which is that the fees are multiplied by
powBase(https://git.duniter.org/clients/cesium-grp/cesium2s/-/blob/develop/src/app/account/accounts.service.ts?ref_type=heads#L483) when in fact they souldn't, as they are in GDcents and not GD (see https://git.duniter.org/clients/cesium-grp/cesium2s/-/blob/develop/src/app/network/network.service.ts?ref_type=heads#L25).
Merge request reports
Activity
requested review from @blavenie
assigned to @txels
added 3 commits
-
47d78285...01f067dc - 2 commits from branch
develop - 6ae94019 - Fees should use the right units
-
47d78285...01f067dc - 2 commits from branch
added 2 commits
added 18 commits
-
ead5c275...2d928948 - 17 commits from branch
develop - 728fe044 - Fees should use the right units
-
ead5c275...2d928948 - 17 commits from branch
139 142 140 143 build: 141 144 extends: .build 142 needs: ["build:env"] 143 145 image: ${CI_BUILD_IMAGE} 144 only: 145 - develop The pipeline was broken for MRs... now it is fixed @blavenie can you review pls?
139 142 140 143 build: 141 144 extends: .build 142 needs: ["build:env"] 143 145 image: ${CI_BUILD_IMAGE} 144 only: 145 - develop 146 146 147 147 build:feature: 148 148 extends: .build 149 149 image: ${CI_BUILD_IMAGE} 150 150 script: 151 151 # Install deps, because it may have changed since the last build image (build from develop) 152 152 - npm install See how build from a branch already execute
npm install. It should not be executed for thedevelopbranch, because thebuild:envtask already create a fresh docker image, with all up to date dependencies. What is wrong here is aonlypart : your merge request was excluded because not infeature/. May be we should addissue/branchs ?Edited by Benoit Lavenier
479 479 throw new Error('ERROR.AMOUNT_NEGATIVE'); 480 480 } 481 481 482 // Remove decimals, in amount and fee 482 // Convert amount to right unit ("cents") 483 483 amount = amount * powBase; 484 if (fee) fee = fee * powBase; Interesting... although while it is true that the docstring mentions decimals, your comment contradicts my observations, which are:
- This optional fee argument, while declared in this function, is not passed in invocations to the function, so it's undefined in practice
- The default fees specified in well known currencies are in cents https://git.duniter.org/clients/cesium-grp/cesium2s/-/blob/develop/src/app/shared/currencies.ts?ref_type=heads#L87
- When checking the blockchain itself (duniter), all transactions are recorded in cents, so I presume that is the unit in which the API operates. I will debug this to confirm.
So for the time being I would even remove the
feeparameter and always use the network values (in cents) to prevent tripping others up.But I see your point, this is not really a bug (it only converts the undefined argument to align the units) but it makes the code confusing and unit mismatch is a potential source of bugs and the variable names aren't helping make out the difference.
Edited by Carles Barrobés
