fix: constructKey meta data mapping

[dolcalmi]

This is a bug found in https://github.com/flash-oss/medici/pull/59 (need to be solved before merge) ~and is a potential breaking change to current implementations of this version~

Problem

balance constructKey does not map correctly the meta object. passing { param1, param2 } and { param2, param1 } create a different key (balance snapshot will not return a valid value)

Solution

add .sort before meta mapping

[Uzlopak]

I dont think this is breaking change. In worst case scenario after an update to new medici version the key gets not found and a snapshot has to be made again. Or do you see some bigger issue?

[dolcalmi]

I dont think this is breaking change. In worst case scenario after an update to new medici version the key gets not found and a snapshot has to be made again. Or do you disagree?

:thinking: really not sure how people is implementing it but.. I think you are right

[Uzlopak]

@koresar @dolcalmi Actually, because we are touching the key generation: I would recommend to sha1 the key to reduce the size of the key. If there is some crazy long query, we reduce it to a 40 digits long string, and this reduces the pressure on the index on mongodb.

[dolcalmi]

@koresar @dolcalmi Actually, because we are touching the key generation: I would recommend to sha1 the key to reduce the size of the key. If there is some crazy long query, we reduce it to a 40 digits long string, and this reduces the pressure on the index on mongodb.

hex or base64 encoding?

[Uzlopak]

encoding latin1 (or the alias binary) should be the way to go . Hex would bloat it up to 80 chars...

[dolcalmi]

encoding latin1

digest supported encodings 'base64' | 'base64url' | 'hex'

maybe we are talking about different things... I will push the change and then I can update it if it is necessary

[Uzlopak]

I checked.. I guess you use node 12 or node 16. In node 14 it is accepting latin1 in digest. But you can just digest to a Buffer and then this buffer has toString(), where you can pass 'latin1', to get the binary string:

here a repl: require('crypto').createHash('sha1').update("bla").digest().toString('latin1')

works in node 12, 14 and 16

[Uzlopak]

btw. latin1 creates a 20 character(= 40 bytes) string and hex creates a 40 character(=80 bytes) string.

[Uzlopak]

I like your changes. Lets see what koresar thinks about it.

[Uzlopak]

I am reading again the code and tbh I think there is even a bigger bug in there. Imagine you have some additional keys on root level. E.g. walletId. Then you maybe do a getBalance, but because you just do book account and meta for snapshotting, you are dropping the information, of the walletId. I think this would break e.g. in galoy. It has to be more generic to respect custom fields too...

@nicolasburtey FYI

[dolcalmi]

I am reading again the code and tbh I think there is even a bigger bug in there. Imagine you have some additional keys on root level. E.g. walletId. Then you maybe do a getBalance, but because you just do book account and meta for snapshotting, you are dropping the information, of the walletId. I think this would break e.g. in galoy. It has to be more generic to respect custom fields too...

@nicolasburtey FYI

that case is fixed here https://github.com/flash-oss/medici/pull/59 (all fields, meta and custom schema attrs are included)

[Uzlopak]

Ok, then lets wait till koresar approves it. I could approve it, but he is the maintainer and should decide what goes into master ;).

[koresar]

Wow! Amazing! I love what you did here people. Let me process this PR in few hours.

[koresar]

Merged to master.