client/{core,db}: add topic IDs to notifications #1197
Conversation
Subjects are not language-dependent, so using the Subject field of Notification to dictate logic is not longer sound. This work adds a "topic" to notifications, which is a language-independent unique ID. The topic takes the place of the subject in the core translation maps. Additionally unexports the Translation and its fields. Client db is upgraded to store the topic, but upgraded clients will have no topics on pre-upgrade notifications retrieved using (*DB).NotificationsN. This is not a problem for us anywhere, and it would be bad practice to do use historical notifications to trigger sensitive operations anyway, so I suspect this won't be a problem for any existing core consumers.
| switch (note.topic) { | ||
| case 'RegUpdate': | ||
| this.updateExchangeRegistration(note.dex, false, note.confirmations) | ||
| break | ||
| case 'Account registered': | ||
| case 'AccountRegistered': |
There was a problem hiding this comment.
This is cool; I just want to note that part of why I suggested a special field for certain note types that demand extra logic is that it would be less fragile than strings hard coded in JS that must match Go. e.g. If it were a flag, the value would be implicit (undefined/true/false). I think the current change here keeps the diff minimal and thus minimizes changes for breakage, so I'm happy with this change, just noting a different strategy for future notifications.
client/core/notification.go
Outdated
| c.log.Errorf("no translation found for topic %q", topic) | ||
| return string(topic), "translation error" | ||
| } | ||
| return trans.subject, c.localePrinter.Sprintf(trans.template, args...) |
There was a problem hiding this comment.
👍 I think. Before, we were using the localerPrinter to look up the actual translations, when using the map like this gives them to us in trans.{subject,template}. Now the locale printer just takes care of number formatting and such for just the template (subject has no need for this having no fmt % specifiers).
I'm just a little uncertain still about what SetString is doing for us any more, unless this is changed to c.localePrinter.Sprintf(topic, arg...) so that it looks up the translated message text in the catalog by the same key used with SetString (topic).
return trans.subject, // already translated and no formatting
c.localePrinter.Sprintf(topic, args...), // localized formatting, key matches SetStringI think we'll probably just need to try these things with @vctt94 's pt PR.
There was a problem hiding this comment.
Oof. You're right. It should be topic.
| func init() { | ||
| for lang, translations := range locales { | ||
| for topic, translation := range translations { | ||
| message.SetString(language.Make(lang), string(topic), translation.template) |
There was a problem hiding this comment.
Since we're now just using the Core.locale map, is it still necessary to build out the message.defaultCatalog anymore via message.SetString?
In this SetString call, key is string(topic), but in localePrinter.Sprintf, key is trans.template, so it looks like it's not even going to be using the catalog/dictionary built here. Poking in golang.org/x/text/message, it looks like using the catalog would be more efficient as SetString pre-compiles the message for the given locale, which presumably would have to be done every time Sprintf is called if not using the catalog (the case when using the matching key).
I'm probably confused about the message package.
There was a problem hiding this comment.
I think I've convinced myself that localePrinter.Sprintf should be using topic as the key to use the catalog we setup with SetString so it finds the pre-compiled message. https://play.golang.org/p/mtvee1t2Pxn
Subjects are not language-dependent, so using the
Subjectfield ofNotificationto dictate logic is not longer sound. This work adds a "topic" to notifications, which is a language-independent unique ID. The topic takes the place of the subject in the core translation maps. Additionally unexportsTranslationand its fields.Client db is upgraded to store the topic, butClient DB is not upgraded. Older notifications will simply go throughdecodeNotification_v0during decoding. Upgraded clients will have no topics on pre-upgrade notifications retrieved using(*DB).NotificationsN. This is not a problem for us anywhere, and it would be bad practice to use historical notifications to trigger sensitive operations anyway, so I suspect this won't be a problem for any existing core consumers.