[ci] Remove /v0/total-amulet-balance and /v0/wallet-balance endpoints#3546
[ci] Remove /v0/total-amulet-balance and /v0/wallet-balance endpoints#3546ray-roestenburg-da merged 10 commits intomainfrom
Conversation
d9b8581 to
291e5fd
Compare
| @@ -67,12 +63,6 @@ class HttpTokenStandardMetadataHandler( | |||
| } | |||
| } | |||
|
|
|||
| private def lookupTotalSupplyByLatestRound()(implicit ec: ExecutionContext, tc: TraceContext) = | |||
There was a problem hiding this comment.
@OriolMunoz-da please check if these changes (and further below in this file) are OK, (ignoring noHoldingFeesOnTransfers) otherwise I have to keep getTotalAmuletBalance around in the store, but I would prefer to just delete more.
There was a problem hiding this comment.
noHoldingFeesOnTransfers.supported is true for all networks now, so you shouldn't need the fallback
the issue is in tests though, because it depends on the latest ACS snapshot which might not be available (as it's computed every 3h)
| @@ -67,12 +63,6 @@ class HttpTokenStandardMetadataHandler( | |||
| } | |||
| } | |||
|
|
|||
| private def lookupTotalSupplyByLatestRound()(implicit ec: ExecutionContext, tc: TraceContext) = | |||
There was a problem hiding this comment.
noHoldingFeesOnTransfers.supported is true for all networks now, so you shouldn't need the fallback
the issue is in tests though, because it depends on the latest ACS snapshot which might not be available (as it's computed every 3h)
| sv1ScanBackend | ||
| .lookupInstrument("Amulet") | ||
| .flatMap(_.totalSupply) | ||
| .valueOrFail("total balance not yet defined") |
There was a problem hiding this comment.
for example here, how do you know that a snapshot was generated?
(maybe there's a forceSnapshot somewhere that I'm not seeing)
There was a problem hiding this comment.
ah, thanks
There was a problem hiding this comment.
always forcing now in getTotalAmuletBalance in ScanAppReference
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Lgtm minus the comments Oriol already made, thank you!
I assume we already discussed this with product (probably while I was out), if not let's ofc do that so noone gets surprised by it.
| sql""" | ||
| select greatest( | ||
| 0, | ||
| sum_cumulative_change_to_initial_amount_as_of_round_zero - |
There was a problem hiding this comment.
we should also be able to cleanup the dbs and txlog parsers further, perhaps in multiple steps:
- make the columns here and in the scan txlog only required for the balances nullable
- stop writing those fields
- eventually nuke those columns
I'd suggest to move this to separate PRs but let's make sure we push through at least 1 and 2 as it meaningfully simplifies the code.
There was a problem hiding this comment.
Good idea, thanks
There was a problem hiding this comment.
I'll add another PR follow up for this.
There was a problem hiding this comment.
I'll do this later though, rather first see what more can be removed and then simply do it all at once.
|
|
||
| - Scan | ||
|
|
||
| - ``/v0/total-amulet-balance`` and ``/v0/wallet-balance`` endpoints have been removed. |
There was a problem hiding this comment.
I suggest mentioning the replacement endpoints here and also mention that the existing endpoints are already deprecated. "deprecated endpoints have been removed in favor of X" just sounds much nicer than "Y has been removed"
|
Also removed wallet-balance usage in scan_txlog.py and everything that was using it (removed option |
7e1cd6b to
c483204
Compare
|
FYI I will not merge this until I get OK from Product (we have discussed before, but to be certain) |
| ) | ||
| def getTotalAmuletBalance(): Option[BigDecimal] = { | ||
| val _ = forceAcsSnapshotNow() | ||
| lookupInstrument("Amulet") |
There was a problem hiding this comment.
any reason not to make this an error instead of an optional? it seems like we really always expect amulet to be present
There was a problem hiding this comment.
yeah can do that, it's only used for testing now anyway
| ) | ||
| def getTotalAmuletBalance(): Option[BigDecimal] = { | ||
| val _ = forceAcsSnapshotNow() | ||
| lookupInstrument("Amulet") |
There was a problem hiding this comment.
nit: read this from whatever config we have for this instead of hardcoding the string
| @@ -26,7 +26,7 @@ | |||
| }, | |||
| "../token-standard/dependencies/canton-json-api-v2/openapi-ts-client": { | |||
| "name": "@lfdecentralizedtrust/canton-json-api-v2", | |||
| "version": "3.4.0-SNAPSHOT", | |||
| "version": "3.4.8-SNAPSHOT", | |||
There was a problem hiding this comment.
not sure I understand why this changed now but it does seem correct 🤷
There was a problem hiding this comment.
I also don't know, I did a npmFix and npmLint at some point..
a9745e9 to
b0ad3c4
Compare
1e57f3a to
d4e02d9
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit 90b485e6f06c872792e27907b008f730a2e4e2ce in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/47691 |
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
…e/ScanAppReference.scala Co-authored-by: moritzkiefer-da <45630097+moritzkiefer-da@users.noreply.github.com> Signed-off-by: Raymond Roestenburg <98821776+ray-roestenburg-da@users.noreply.github.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
90b485e to
0eb5150
Compare
Fixes #3503
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines