Support canonical ledger entry names #5271
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5271 +/- ##
=======================================
Coverage 78.1% 78.1%
=======================================
Files 790 790
Lines 67556 67557 +1
Branches 8158 8155 -3
=======================================
+ Hits 52785 52793 +8
+ Misses 14771 14764 -7
... and 3 files with indirect coverage changes |
|
Why deprecate the old filter names? Why not support both? |
@mDuo13 can you respond? In the issue you had raised you mentioned "For backwards compatibility, continue to also support the existing short names (but mark them deprecated in the documentation)." - are there requests from the community to deprecate that field to reduce confusion, or is there another reason? |
|
The reason to mark it deprecated is so that we can move away from needing a short name for new ledger entries (a dev task that was often forgotten in the past anyway). There should be one preferred way to look up ledger entry types, by their canonical name (case-insensitive). It's fine if the existing, "deprecated" shortcut names are supported indefinitely too. |
|
Maybe this is an unpopular opinion but I actually like the lower-case names, and think we should keep them. note: I have no objection to adding additional filter names, just to deprecating/replacing the existing setup. |
I have no particular opinion here or sufficient background to know how people use this API, but do note that this PR explicitly adds support for case-insensitive filtering; so lowercase names are definitely possible. |
| static constexpr auto types = | ||
| std::to_array<std::pair<char const*, LedgerEntryType>>({ | ||
| static constexpr auto types = std::to_array< | ||
| std::tuple<char const*, char const*, LedgerEntryType>>({ |
There was a problem hiding this comment.
nit: For readability, consider a small struct instead of the tuple.
There was a problem hiding this comment.
I figured that if we're going to deprecate the current way of referencing ledger entries, then the next release we can return to using a std::pair - which would be preferred over a dedicated struct.
@mDuo13 & @mvadari, let's discuss whether or not to deprecate the existing setup. In particular, @mDuo13 raised some good points, so @mvadari do you feel strongly about that we should keep the existing, frequently odd, naming convention & the increased burden in remembering to add such a separate name for each new ledger entry type? |
The |
High Level Overview of Change
This change implements #4393, which enhances the filtering in the
ledger,ledger_data, andaccount_objectsmethods by also supporting filtering by the canonical name of theLedgerEntryTypeusing case-insensitive matching.Context of Change
Currently the mapping between the RPC name to provide as filter and the
LedgerEntryTypeis inconsistent and sometimes surprising. For example, to match theDirectoryNodethe provided RPC name isdirectory, while forPayChannelit ispayment_channel.This change enables filtering on the canonical name as well to make the mapping consistent and unsurprising. For instance, to filter on
PayChannelit is still possible to specifypayment_channel, but now it's also possible to specifyPayChannelandpaychannel(or any variant thereof using different casing).Type of Change
API Impact
Before / After
See #4393 for an excellent description.