Conversation
PR #421 Review: POS Mode with TX WatchSummary: Adds a real-time transaction watch to the POS flow so the merchant sees payment confirmation in <1s (from the tx pool, before block finality). Also upgrades Rust crypto crates to match the new Planck testnet, changes the HD derivation path to fully hardened, and adds dev account handling. OverallThis is a solid feature. The WebSocket subscription to the tx pool for instant payment feedback is a great UX win for POS. The Rust crate upgrades are necessary for the new testnet. The code is clean and focused. That said, there are some things worth flagging: Issues1. Note: Not really an issue... crypto is precise and >= isn't a lot better than == final expectedPlanck = _fmt.parseAmount(widget.amount);
// ...
if (expectedPlanck != null && received == expectedPlanck) {The payment is only matched when the received amount is exactly the expected planck value. If the sender's wallet formats differently, has a rounding variance, or the user manually types a slightly different amount, it won't match. This is probably fine for the MVP "trusted parties" use case, but worth noting. Consider whether a 2. No Note: We don't know the customers address.. The 3. 30-second timeout silently stops watching Note:
_timeoutTimer = Timer(const Duration(seconds: 30), () {
print('[TxWatch] Timeout — gave up waiting for payment');
_txWatch.dispose();
if (mounted) setState(() => _watching = false);
});After 30s, the watcher stops and the UI changes from "Waiting for payment..." back to a plain "Done" button. The user gets no feedback that monitoring stopped. If the payment arrives at second 31, it's missed. Consider either extending this timeout, showing a "Timed out — tap to retry" message, or re-subscribing on tap. 4. WebSocket error handling is print-only Note:
In 5. Intentional, we know. final derivationPath = "m/44'/189189'/$account'/$change'/$addressIndex'";The old path was The Rust side also adds a This path is hardcoded in Rust and in Dart independently. If they diverge, 6. Note:
_ws!.listen(
(event) { ... },
onError: (e) { ... },
);The 7. Dev account seeds are hardcoded in two places Note: Fix.
8. Note: Non-issue -int get publicKeySize =>
- RustLib.instance.api.crateApiCryptoPublicKeyBytes().toInt();
+BigInt publicKeyBytes() => RustLib.instance.api.crateApiCryptoPublicKeyBytes();This changes the public API from 9. [patch.crates-io]
ark-vrf = { git = "https://github.com/davxy/ark-vrf", tag = "v0.1.1" }The comment says the crate was yanked. Patching to a git tag is fine as a workaround, but this should be tracked as tech debt — if 10. Rust toolchain pinned to Pinning is good for reproducibility. Just make sure CI also uses this version. Minor / Nits
What's Good
Summary VerdictThe feature works and is well-scoped for a trusted POS scenario on a new testnet. The main things I'd want addressed before merge are:
Everything else is minor or acceptable tech debt for a testnet feature branch. |
| return httpUrl.replaceFirst('https://', 'wss://').replaceFirst('http://', 'ws://'); | ||
| } | ||
|
|
||
| Future<void> watch({ |
There was a problem hiding this comment.
Shouldn't we also use healthiest url here?
| _buildQrCode(request.paymentUrl, colors), | ||
| const SizedBox(height: 16), | ||
| const SizedBox(height: 12), | ||
| // GestureDetector( |
There was a problem hiding this comment.
Why do we have commented line here?
dewabisma
left a comment
There was a problem hiding this comment.
Just need to clarify this, all good
Note: This unfortunately requires new testnet.
I think we should make a branch of main for this.
Update: Targeting new_testnet_planck - which is our new app running on planck