Deterministic Aliases#284
Conversation
|
Functional test - seems to work as intended. You'll need to revise the 'Offline Handshake' flow though as it's now broken. Whether we remove this and absorb it as part of the discrete mode - makes sense) - but this hidden mode is far 'cleaner' than the current new chat window (speaking UI here). So maybe some consolidation needs to be done. Also, adding a discrete chat should probably still show a message or something that handshake is completed? Maybe something like "Discrete Chat Initiated". Also, I'm still not sold on these docs being part of the repo. |
|
^ Edited above. |
|
Also FYI, maybe a little more detail in the PR desc next time. |
|
**"You'll need to revise the 'Offline Handshake' flow though as it's now broken. Whether we remove this and absorb it as part of the discrete mode - makes sense) - but this hidden mode is far 'cleaner' than the current new chat window (speaking UI here). So maybe some consolidation needs to be done."**
"Also, adding a discrete chat should probably still show a message or something that handshake is completed? Maybe something like "Discrete Chat Initiated"."
"Also, I'm still not sold on these docs being part of the repo."
|
I should've specified... I don't think the 'hidden-ness' is cleaner - rather the model itself. |
|
Alright, |
Decent idea. Maybe let's leave it as is, do the clean up and the review of core idea and after I'm happy to propose some UI work. |
…andshake logic bug
|
@HocusLocusTee |
|
Ok added a toast for the discrete message init, but will change it to a message in the thread. What do you mean by "self stash the alias"? |
In the current system we send a 'self stash' tx to ourselves - which is indexed by the indexer - so we can keep the handshake between difference devices. Now, in the proposed alias changes: if I am |
|
@HocusLocusTee Also something I noticed that would be a useful future improvement; seems like contact names/nicknames are not stored anywhere except the local db. Should probably track these on the indexer using some sort of similar self-stash method. |
|
"If A has no funds, they cannot start Discrete chat. (They should be able to). We should find a way to segregate the self stash tx and the creation of chat. See old Offline Handshake flow." Not sure I understand this. |
|
Self stash does require KAS. But setting up a discrete handshake does not. Right now, no balance blocks me from even creating the handshake. If balance is 0, we could automatically skip the self stash... but we should eventually add a way to stash it again at a later date. |
… handshaking an emepty wallet (now always listening when initiatedByMe is true
|
Got it - self stash is now skipped when funds are not available. |
| }); | ||
| // Optionally create self-stash for cross-device sync (requires minimal funds for network fees) | ||
| // Check if user has sufficient balance before attempting | ||
| const minFeeAmount = BigInt(100000); // ~0.001 KAS for network fees |
There was a problem hiding this comment.
we use const minAmount = kaspaToSompi("0.19"); in other places. maybe we should generalize this.
There was a problem hiding this comment.
Leaving this as is for now - linter flags usage in the subsequent conditional if I change it to use kaspaToSompi since undefined can be returned.
| import { ImagePresenter } from "../Modals/ImagePresenter"; | ||
| import { BroadcastParticipantInfo } from "../Modals/BroadcastParticipantInfo"; | ||
| import { QrScannerModal } from "../Modals/QrScannerModal"; | ||
| import { OffChainHandshakeModal } from "../Modals/OffChainHandshakeModal"; |
There was a problem hiding this comment.
We need to clean up OffChainHandshakeModal.
| }) => { | ||
| const handleShortPress = () => { | ||
| onNewChat(); | ||
| const handleClick = () => { |
There was a problem hiding this comment.
We don't need this component at all anymore. It can be styled with a regular + button and none of the 'holdable' complexity.
There was a problem hiding this comment.
I had left it for future usecases, but will remove
| @@ -0,0 +1,306 @@ | |||
| # Debugging Alias Routing - Logging Added | |||
There was a problem hiding this comment.
I still dont think we need this doc. There are plenty of decent comments explaining how this works in the new util file.
| } | ||
| } else { | ||
| console.log( | ||
| "[createDiscreteConversation] Skipping self-stash - insufficient balance for network fees (conversation works locally)" |
There was a problem hiding this comment.
Great. But how does one self stash at a later date?
We should add a warn that we did not self stash due to no balance.
We also need to add the ability to self stash at a later date (and track this somehow). I don't think it should be part of this PR as it'll muddle the context - but we should be aware and open an issue / put it into future work.
There was a problem hiding this comment.
Want me to open this as a new issue? Or can you?
|
@MEZTech-LLC please see the commits in this branch: Feel free to cherry-pick them if you approve. |
|
Looks good to me |
| shouldSendResponse: boolean; | ||
| conversationId: string; | ||
| }; | ||
| if (typedResult.shouldSendResponse) { |
There was a problem hiding this comment.
I think we should extract some of this to the conversation manager stop. Maybe something like sendAutomaticHandshakeResponse
| // add an offline handshake | ||
|
|
||
| /** | ||
| * @deprecated Manual alias exchange is no longer needed. Aliases are derived deterministically. |
There was a problem hiding this comment.
Probably dont need legacy comments like this and the one on line 1247
| this.monitoredAddresses.clear(); | ||
| const conversations = conversationManager.getMonitoredConversations(); | ||
|
|
||
| console.log("[block-processor] Updating monitored aliases:", { |
There was a problem hiding this comment.
I think we should remove the logs you've added. They are way too noisy.
There was a problem hiding this comment.
@MEZTech-LLC I've done it for you. feel free to just pick the last 2 commits so I don't claim write ownership over your other commits
#289
There was a problem hiding this comment.
Thanks @HocusLocusTee - Apologies I couldn't get to this sooner; other pressing tasks came up
IzioDev
left a comment
There was a problem hiding this comment.
Rust part sounds good, I'll dedicate more time on the application code later (and actually try it locally too :) ).
| // Get my public key from my private key | ||
| let my_public_key = my_private_key.to_public_key() | ||
| .map_err(|_| JsError::new("Failed to derive public key"))?; |
There was a problem hiding this comment.
Minor: We can avoid computing pk from sk if the client provide a public key instead of the private key.
Public key can safely be computed once by the client upon startup (where we initiate the wallet state), and provide it here.
same for xonly_pk (below)
| // Parse their address to extract their public key | ||
| let address = Address::try_from(their_address) | ||
| .map_err(|e| JsError::new(&format!("Invalid address: {}", e)))?; | ||
|
|
||
| // Extract X-only public key from address (32 bytes, no parity) | ||
| let their_xonly_pk = XOnlyPublicKey::from_slice(address.payload.as_slice()) | ||
| .map_err(|e| JsError::new(&format!("Invalid public key in address: {}", e)))?; |
There was a problem hiding this comment.
This block is done twice, once here and once in derive_alias_with_context.
I propose to let responsibility to calling site to compute address + xonly_pk and now derive_alias_with_context takes a xonly_pk as argument
|
|
||
| // Construct info string: "chat" || shared_secret || context_pubkey | ||
| let mut info = Vec::new(); | ||
| info.extend_from_slice(b"chat"); |
There was a problem hiding this comment.
If we re-use another domain separation, maybe we should name chat more explicitly.
What it is, is: a 1:1 conversation alias for now. I propose to simplify as alias as it may be a general concept within the application for other purpose as-well
There was a problem hiding this comment.
or dm_alias, or pchat (private chat) or w/e
nit: could be made under an enum: enum HashDomain { ... }
# Conflicts: # src/components/Modals/NewChatForm.tsx # src/service/block-processor-service.ts # src/service/conversation-manager-service.ts # src/store/ui.store.ts
| // In deterministic system: I monitor myAlias, they send to theirAlias | ||
| // Due to ECDH symmetry: their theirAlias === my myAlias | ||
| monitored.push({ | ||
| alias: conversationAndContact.conversation.myAlias, |
There was a problem hiding this comment.
This is correct, but will be an issue with old v1 alias. Do we just optimistically hope they upgrade? If so, why do we then even give them the option to upgrade if the code isn't fully compatible with v1.
There was a problem hiding this comment.
True, maybe just lock the conv and force manual upgrade (no hidden / automatic mechanism), wdyt?
There was a problem hiding this comment.
Is it with just running a db migration on boot?
There was a problem hiding this comment.
Currently, on boot it adds a version field at conversation level (the entity holding alias data) and we set it to v1 by default.
Later, when you enter a conversation, if it's v1, you're asked to migrate (it replays handshake, and set to v2).
All new conversation are created as v2
There was a problem hiding this comment.
Ack on that. But why not just compute new alias on db bump / opening new version?
There was a problem hiding this comment.
Absolutely right, 0 need into re-initiating an handshake. Besides it would reveal stealth conversation.
There was a problem hiding this comment.
Should be good with latest commit, let me know what you think, silently migrate basically
| <Button | ||
| variant="primary" | ||
| onClick={sendUpgradeResponse} | ||
| disabled={isSubmitting} |
There was a problem hiding this comment.
Maybe we should disable this with a funds check and a message if they try to send with no funds?
There was a problem hiding this comment.
General Q for this great feature: Wonder if we need a way to 'force' an upgrade. Lets say they are not getting upgrade messages, we can compute this without interaction, so should we allow it?
There was a problem hiding this comment.
Removed because I don't think it has a use-case at the moment, maybe for dcom upgrade (not even sure that'll be necessary)


New implementation (deterministic, but non-unified aliases).
Distilled related documentation.