-
Notifications
You must be signed in to change notification settings - Fork 171
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/ledgerhq/ledger-live-common/ky12u3u76 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some feedback on different places. my main feedback is to finish the work done in the specs.js, at least we should improve the bot tests because dataset.js feels pretty light at the moment in term of coverage (one account, some txs).
For the bot i feel we should have at least covered:
- sending assets
- validation of text on the device
- claimReward (with some invariant on exactly what "selects" a claim reward tx candidate, like from a certain available amount – see cosmos)
a bit less prio but still recommended would also to cover:
- memo
- opt out only if it's technically easy to add: even if it's not part of Live UI, i feel the bot needs it for the duality of actually being able to test opt in 🤔
@@ -0,0 +1,151 @@ | |||
/******************************************************************************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there plan to contribute back to ledgerjs repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, but it can be, but I don't know what the requirement to be in ledgerjs repo ? what the minimum functionalities we do need ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can deal with it after it's merged, but basically we should make a @ledgerhq/hw-app-algorand
with what you have so the community can reuse / contribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we test on mobile? I have strong suspicion there will be bugs, at least with this present feedbacks
The PR is still not mergeable, we are still missing :
the libcore PR to be merged
Algorand Integration lib-ledger-core#616
Then, update his bindings for desktop:
Node Bindings for Algorand integration lib-ledger-core-node-bindings#82
and mobile:
React Native Bindings for Algorand integration lib-ledger-core-react-native-bindings#61
add a new nano for the test-dataset
enable bot