Bugfix for Hash/Secret lock transactions with Delegated Harvesters (task#5pm2hb) - #241
Bugfix for Hash/Secret lock transactions with Delegated Harvesters (task#5pm2hb)#241ruellm wants to merge 3 commits into
Conversation
… Added version in LockInfo entry and notifications. Upgraded version of Hashlock and secret lock to support version 2
…version attribute.
| "name": "Hash_Lock", | ||
| "type": "16712", | ||
| "supportedVersions": [1] | ||
| "supportedVersions": [1, 2] |
There was a problem hiding this comment.
Let's support only version 2=)
| sub.notify(HashLockMosaicNotification<1>(transaction.Mosaic)); | ||
| sub.notify(BalanceDebitNotification<1>(transaction.Signer, transaction.Mosaic.MosaicId, transaction.Mosaic.Amount)); | ||
| sub.notify(HashLockNotification<1>(transaction.Signer, transaction.Mosaic, transaction.Duration, transaction.Hash)); | ||
| sub.notify(HashLockNotification<1>(transaction.Signer, transaction.Mosaic, transaction.Duration, transaction.Hash, transaction.EntityVersion())); |
There was a problem hiding this comment.
I agree with you that this solution is more beauty, but I'm not sure that we need to do changes related to transaction's version by different way.
I think will be better do it by ugly way with the same handlers but for another version of notification.
Because this variant allow us to have the same steps when we are doing any change like this(like tempalte how to do changes related to versioning=) ).
But if @otsybizov agree with beauty variant, then I agree too=)
There was a problem hiding this comment.
I agree with adding version field to notification structure. But I don't agree with the changes in the lock info serializer. You could use the 2nd version of the serializer rather than changing the 1st version. Having 2 different serializers for the lock entry version 2 is error-prone solution, potentially someone can use invalid serializer and damage the cache.
There was a problem hiding this comment.
@otsybizov thank you for your feedback, I have a question.
just to confirm your suggestion, instead of changing verison 1 of LockInfo serialzer, we will use the second version instead defined here ?
does this mean I will change hashlock info serialzer to use this as well? change this line
how will the first version entry be handled?
There was a problem hiding this comment.
Now, as we have two versions of hash/secret lock info, this becomes more complicated. In both plugins you need to first read the version and then call appropriate serializer when loading entries from cache. And similarly, depending on the entry version, call appropriate serializer when saving data to cache. I'd suggest to add a new LockInfoSerializer template scecialization (version 3) that does that.
4 participants
… Added version in LockInfo entry and notifications. Upgraded version of Hashlock and secret lock to support version 2
change notes:
Also adding new version of notification means adding new observer for notification which only differs in setting the version of the cache entry, it will be a redundant observer because both old and new observer will have to set version anyway.
Also, Version information is already stored in cache anyway, we just need a variable in the chain that handles it.