Skip to content

Conversation

@bznein
Copy link
Collaborator

@bznein bznein commented Nov 24, 2025

Before asking for reviews, here is a check list of the most common things you might need to consider:

  • updating the Changelog
  • writing unit tests
  • checking if your changes affect other coins or tokens in unintended ways
  • testing on multiple environments (Qt, Android, ...)
  • having an AI review your changes

@bznein bznein requested review from benma and thisconnect November 24, 2025 11:43
@bznein
Copy link
Collaborator Author

bznein commented Nov 24, 2025

Adding @thisconnect mostly for the test logic/ts code and @benma for the aoppserver (vibe-coded with ChatGPT, so please feel free to tear it apart).

Copy link
Collaborator

@thisconnect thisconnect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a small nit about data-testid and a question/comment.

Deferring to @benma for the aopp/server.py test backend

@bznein bznein marked this pull request as ready for review November 24, 2025 12:15
await test.step('Send RBTC to receive address', async () => {
await page.waitForTimeout(2000);
const sendAmount = '10';
await sendCoins(recvAdd, sendAmount);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is only 1 account the aopp workflow will have one less step (the step with the account selector).

Similarly to this: if the device is not yet connected&unlocked the aopp overlay will close and only show a small single banner "Address request in progress. Please connect your device to continue.
"
... on the waiting view (or if watch-only is active on top of the account-summary). After device unlock the app jumps back into the AOPP workflow.

Both cases sounds like something that could be tested here, but maybe it would make the tests much more complicated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they are both sensible things to test! I'll definitely add both

@bznein bznein force-pushed the aoppE2Etest branch 2 times, most recently from 3c45cad to 8547a65 Compare November 25, 2025 15:03
Copy link
Collaborator

@thisconnect thisconnect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you 🙏

LGTM

Comment on lines 94 to 97
sudo apt-get update
sudo apt-get install -y build-essential libffi-dev libssl-dev libsecp256k1-dev python3-dev
pip install --upgrade pip
pip install flask coincurve bech32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these tests be run inside Docker? If so, should we add this to the Dockerfile?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need something more to be able to run these tests in Docker (for example, I don't think we can run the regtest script inside docker 😅). I think it would be worth trying but out of scope for this PR, wdyt?

msg = uri[msg_start + 4:] if msg_end == -1 else uri[msg_start + 4:msg_end]

sig_bytes = proof.signature
if len(sig_bytes) == 64:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and else? 😄

import uuid
import hashlib
import base64
from coincurve import PublicKey
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using the ecdsa library, it's what we use in most other tooling and is also available in the Docker image already. It has support for the secp256k1 curve.

@bznein bznein force-pushed the aoppE2Etest branch 4 times, most recently from efb7832 to 8de186f Compare December 1, 2025 16:31
@bznein
Copy link
Collaborator Author

bznein commented Dec 1, 2025

@benma I did a big refactor of the aopp server, based on your feedback and the fact that it was actually broken 😅 Can you please take another look? thanks!

@bznein bznein requested a review from benma December 1, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants