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

Multiassets #54

Merged
merged 10 commits into from
Feb 18, 2021
Merged

Multiassets #54

merged 10 commits into from
Feb 18, 2021

Conversation

janmazak
Copy link
Collaborator

Not 100% finished and needs some polishing.

@janmazak janmazak requested a review from refi93 January 28, 2021 01:13
src/signTx.c Outdated Show resolved Hide resolved
src/signTx.c Outdated Show resolved Hide resolved
src/signTx.c Outdated Show resolved Hide resolved
src/signTx.c Outdated Show resolved Hide resolved
src/signTx.h Outdated Show resolved Hide resolved
src/signTxOutput.c Outdated Show resolved Hide resolved
src/signTxOutput.c Outdated Show resolved Hide resolved
src/signTxOutput.c Outdated Show resolved Hide resolved
src/textUtils.c Outdated Show resolved Hide resolved
src/txHashBuilder.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@refi93 refi93 left a comment

Choose a reason for hiding this comment

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

see comments

@janmazak janmazak force-pushed the multiasset branch 2 times, most recently from da7747a to b412350 Compare January 30, 2021 09:49
@janmazak janmazak requested a review from refi93 February 2, 2021 21:02
src/uiScreens.c Outdated Show resolved Hide resolved
src/txHashBuilder_test.c Outdated Show resolved Hide resolved
src/cardanoOutputs.h Outdated Show resolved Hide resolved
src/securityPolicy.c Outdated Show resolved Hide resolved
src/signTx.c Outdated Show resolved Hide resolved
src/signTxOutput.c Outdated Show resolved Hide resolved
src/txHashBuilder.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@refi93 refi93 left a comment

Choose a reason for hiding this comment

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

see comments

src/uiScreens.c Show resolved Hide resolved
src/textUtils.h Show resolved Hide resolved
HANDLE_VALIDITY_INTERVAL_START_STEP_INVALID,
};

static void signTx_handleValidityInterval_ui_runStep()
Copy link
Collaborator

@refi93 refi93 Feb 3, 2021

Choose a reason for hiding this comment

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

UX-wise I find it a bit counterintuitive that validity interval start is being confirmed after TTL, but I guess reversing the order is not really an option given that it's coupled with tx serialization :/

I'm wondering if it would not make sense to (eventually) move ttl/validity start directly to the init APDU to make the UX around those values more intuitive and remove that "machinery" just for two numbers

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 don't understand why they did not just replace ttl with validity interval (in the same way as coin in output is replaced by coin / [coin, multiasset]). That would fix all ui problems and also look more natural.

On one hand, we don't have much time to do changes; on the other, why show the users the "wrong" user flow with validity interval if we can avoid it?...

@refi93
Copy link
Collaborator

refi93 commented Feb 5, 2021

@janmazak don't forget to update the docs: https://github.com/vacuumlabs/ledger-app-cardano-shelley/blob/develop/doc/ins_sign_tx.md so they don't fall out of sync

@refi93 refi93 merged commit c109fca into develop Feb 18, 2021
@janmazak janmazak deleted the multiasset branch June 29, 2021 14:20
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