Skip to content
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

feat: disable submit button for masp tx when using disconnected ledger device #1572

Conversation

mateuszjasiuk
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk commented Jan 20, 2025

quit app -> open app -> unplug device -> plug device in and open app again

ledger.button.mp4

@mateuszjasiuk mateuszjasiuk force-pushed the feat/shielded-transfers-using-disposable-gas-payer branch 3 times, most recently from 3bfbb29 to 1c6d586 Compare January 27, 2025 14:40
Base automatically changed from feat/shielded-transfers-using-disposable-gas-payer to feat/ledger-masp-integration-branch January 30, 2025 15:57
@mateuszjasiuk mateuszjasiuk force-pushed the feat/disable-submit-button-for-masp-tx-when-ledger-is-not-connected branch from e8afcf7 to 029ea71 Compare January 30, 2025 16:02
@mateuszjasiuk mateuszjasiuk changed the title feat: disable submit button for masp tx using disconnected ledger feat: disable submit button for masp tx when using disconnected ledger device Jan 30, 2025
@mateuszjasiuk mateuszjasiuk force-pushed the feat/disable-submit-button-for-masp-tx-when-ledger-is-not-connected branch from 029ea71 to 55aca55 Compare January 30, 2025 16:13
@mateuszjasiuk mateuszjasiuk requested review from euharrison, pedrorezende and jurevans and removed request for euharrison, pedrorezende and jurevans January 30, 2025 16:14
@mateuszjasiuk mateuszjasiuk marked this pull request as ready for review January 30, 2025 16:15
return {
refetchInterval: 1000,
queryKey: ["ledger-status"],
queryFn: async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think something may need to change here after testing. What I found was that this loop continues to execute after I've connected and submitted a Tx, and I hit a few cases with different errors thrown. It will definitely be an issue if we try to open transport on Web when the Keychain is attempting to open transport to sign. I'll give it some thought! Maybe if we, say, detect it once, if it's undetected, require the user to click to Connect, once connected, fetch build params then submit. I think maybe it would be better if validation happens once, or, if the check is discontinued after first successful connection? Not sure, will take some thought

@jurevans
Copy link
Collaborator

jurevans commented Jan 30, 2025

@mateuszjasiuk looks good! I did encounter some weird behavior when trying to Unshield, I think related to the loop and transport management. I think it's leading to edge cases like these:

Namadillo:

TransportInterfaceNotAvailable: Failed to execute 'claimInterface' on 'USBDevice': The device must be opened first.
InvalidStateError: Failed to execute 'close' on 'USBDevice': An operation that changes interface state is in progress.

Keychain:

Uncaught (in promise) NetworkError: Failed to execute 'releaseInterface' on 'USBDevice': Unable to release interface.

We might want to require the user to initiate connection to Ledger to have better management of transport open & close, or ensure that transport is closed on Namadillo by the time it's needed in the Keychain. We can chat about it though!

Copy link
Collaborator

@jurevans jurevans left a comment

Choose a reason for hiding this comment

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

Code looks good, retested the Unshielding tx, and is working great with Ledger!

@mateuszjasiuk mateuszjasiuk merged commit a7567d8 into feat/ledger-masp-integration-branch Jan 31, 2025
7 checks passed
@mateuszjasiuk mateuszjasiuk deleted the feat/disable-submit-button-for-masp-tx-when-ledger-is-not-connected branch January 31, 2025 10:42
mateuszjasiuk added a commit that referenced this pull request Feb 10, 2025
…r device (#1572)

* feat: disable submit button for masp tx using disconnected ledger

* fix: do not check for conencted ledger while tx is submitting
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.

2 participants