-
-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split lightning address settings into global and pubkey specific #2752
base: master
Are you sure you want to change the base?
Conversation
lightningAddress.enabled && | ||
SettingsStore.settings.lightningAddressByPubkey?.[ | ||
NodeInfoStore.nodeInfo.identity_pubkey | ||
]?.enabled && | ||
BackendUtils.supportsCustomPreimages() && | ||
!NodeInfoStore.testnet | ||
) { | ||
if (connecting) { | ||
try { | ||
await LightningAddressStore.status(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if status()
is really needed here, it is a lot slower than the new checkLightningAddressExists()
.
Either way, this can be optimized since status()
and checkLightningAddressExists()
have overlapping functionality (both ask zeuspay backend for LN address).
pubkey: this.nodeInfoStore.nodeInfo.identity_pubkey | ||
}) | ||
), | ||
timeout(10000) // 10 seconds should be enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because migration method migrateLightningAddressSettings()
also calls this and it will hang if backend doesn't answer. 10s should be more than enough imo...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm there's gotta be a better way to handle that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if we want correct data after migration, we have to look up if ln address exists for current pubkey. Since before enabled
was a global setting, we cant rely on that.
Or do you mean how to achieve the timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we run this one in series after the other migrations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not possible because at that point (during getSettings()
) we don't have the node pubkey yet. That's why I call migrateLightningAddressSettings()
in fetchData()
right after await NodeInfoStore.getNodeInfo();
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not so bad really, checkLightningAddressExists()
only runs once per node. Not even that actually, it is skipped if that pubkey was checked already. And chances are low that backend is not responding too...
routeHints: false, | ||
allowComments: true, | ||
nostrPrivateKey: '', | ||
nostrRelays: DEFAULT_NOSTR_RELAYS, | ||
notifications: 0 | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking about this now, we may want to pair these with each address, otherwise it is really easy to get out of sync.
Alternatively, we don't store it at all and and modify the service to return these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, before this PR, it is often out of sync, because it is a globl setting.
I didn't think about pulling it from backend too. Not a bad idea! Let me know, then I'll add that.
7e4d1d8
to
78a48db
Compare
78a48db
to
124a5f9
Compare
rebased/solved conflicts |
Description
(#2687 needs to be merged first.)
Right now we have all lightning address settings as global settings, but
enabled
andnostrPrivateKey
need to be pubkey-specific (not node specific, that is the tricky part). Since we can only obtain the pubkey after we are connected to the node, the migration for this (migrateLightningAddressSettings()
) is called aftergetNodeInfo()
in Wallet.tsx (not like other migrations which are called ingetSettings()
).Before,
LightningAddressStore:status();
was often times not called at all in Wallet.tsx:fetchData(), because the flagenabled
(for LightningAddress) wasfalse
globally (and never corrected). Now it will be correct (or corrected automatically) for all nodes (pubkeys).To make sure there won't be any conflicts/mixed data, it is ensured the migration only runs after other migration methods are finished.
Additionally:
legacySettingsMigrations
andstorageMigrationV2
) again if already running (getSettings()
is called 2x in quick sequence)LEGACY_ADDRESS_ACTIVATED_STRING
andADDRESS_ACTIVATED_STRING
and unused methodgetLightningAddressActivated()
in LightningAddressStore.ts.This also fixes #2730.
This pull request is categorized as a:
Checklist
yarn run tsc
and made sure my code compiles correctlyyarn run lint
and made sure my code didn’t contain any problematic patternsyarn run prettier
and made sure my code is formatted correctlyyarn run test
and made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarn
after this PR is merged inpackage.json
andyarn.lock
have been properly updatedOther: