-
Notifications
You must be signed in to change notification settings - Fork 121
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/253 Implement ledger service in Extension #295
Conversation
213deed
to
6a409cf
Compare
0274838
to
474b48b
Compare
8ea3e0a
to
30d1e5d
Compare
ea7ba23
to
57c4374
Compare
91ef63b
to
fb924af
Compare
fb924af
to
de1fb64
Compare
b7cdd90
to
d85616f
Compare
78fcd03
to
2de4058
Compare
2946257
to
4a72273
Compare
c37f05b
to
8edf4d3
Compare
8edf4d3
to
f7b31f5
Compare
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 think this looks good. Left few comments/questions. I'm curious if it is possible to have one flow instead of two for submitting transactions. So for example we get rid off the submit_bond and we use only submit_signed_bond. The only difference is that we are asking sdk to sign instead of ledger. This would potentially simplify few things :)
); | ||
|
||
// Lock current wallet keyring: | ||
await requester.sendMessage(Ports.Background, new LockKeyRingMsg()); | ||
|
||
// Fetch accounts for selected parent account | ||
await onSelectAccount(account); | ||
onSelectAccount(account); |
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'm curious why did we delete await here I'm curious if it is not problematic in a case that onSelectAccount throws error?
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 removed it because it's not an async function (throwing lint warning)
<Route | ||
path={TopLevelRoute.ConfirmLedgerBond} | ||
element={ | ||
<ConfirmLedgerBond |
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.
Does it mean that we will have confirmation screens for every tx type, times 2 for the ledger ones?
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.
This is what I'm refactoring next - I don't want to have individual views for each tx type necessarily, the only difference we'll encounter is the type of details we display to the user. That's something I'm planning to clean up before implementing other Tx!
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'm thinking I will start by ignoring differences in Tx (reducing the query string that opens the approval window) and keeping common values (amount
, type
, source
) for now, then incorporate differences later as needed. That should make for much cleaner code
const id = query.get("id") || ""; | ||
const amount = query.get("amount") || ""; | ||
const source = query.get("source") || ""; | ||
const tokenAddress = query.get("token") || ""; |
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'm curious if something like:
const {
type,
id,
amount,
source,
token: tokenAddress,
publicKey,
} = query.getParams(["type", "id", "amount", "source", "token", "publicKey"]);
Would be a good thing to add in future.
setStatus(Status.Completed); | ||
} catch (e) { | ||
console.warn(e); | ||
const ledgerErrors = await ledger.queryErrors(); |
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.
Not sure if this can happen, but what if queryErrors gets rejected?
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.
The only error that can happen is that the Ledger instance can't find NamadaApp
- so either the app is not loaded, or the Ledger isn't plugged in and connected. The query for status()
in background/ledger/ledger.ts
catches that situation there, this is more of a wrapper function for status()
that returns any non-error error messages so the client doesn't have to do that logic. However, if that error was thrown in the Ledger
status call, it isn't getting caught here, which isn't good. Maybe it should be moved to the above try
so that the status error, if thrown, will be caught.
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.
Also, I just realized that this is ConfirmLedgerTransfer
- in my next PR I am getting rid of this file entirely - I took out the queryErrors
call in ConfirmLedgerBond
, so I'll leave this as is! Sorry I thought this was the other confirmation!
|
||
const { source, nativeToken, amount: amountBN } = txDetails; | ||
const amount = new BigNumber(amountBN.toString()); | ||
// TODO: This query should include perhaps a "type" indicating whether it's a bond or unbond tx: |
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 think those todos(also line 69) are already done right?
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 think for type
I will make it a part of the routing, so we can just direct it by URL to catch that param, but the type
I've added here is account type, not tx type - it needs to be more explicit because it's confusing!
Re-enable test Fix failing test feat/335 - Approve Bond Tx, Sign bond with Ledger (#336) * Hook up bond approval and Ledger signing * Clean up, fix naming * Add query for public_key * Hooking up Reveal PK tx to extension * Put public_key into explicit schema type * Fix schema order * Refactor submit calls to include signing * Fix error on submit public key reveal, clean up logs * Begin hooking up staking update events * Fix issue with Reveal PK tx
ac0c069
to
d8725ae
Compare
Resolves #253
Resolves #335
This PR adds the Ledger service to the extension, and adds Bond Tx to the approval process, allowing user to sign a Bond Tx with their Ledger (along with Reveal Public Key tx)
Nice-to-have
NOTES
ledger-namada
package inpackages/
, which contains the necessary changes on the client-side to parse Ledger responses, so we are no longer using the@zondax/ledger-namada
package.Testing
Setup
v0.18.1
, do not build wasm scripts! We want the wasms that are delivered through AWS (and remember to fix the CORS setting in the chain'sconfig.toml
, e.g.,local.c4463480b0eb1dbf83a1d0d0/setup/validator-0/.namada/local.c4463480b0eb1dbf83a1d0d0/config.toml
), then start that chain and set up a wallet (CLI)./installer_s2.sh load
(e.g., if you're using a Ledger Nano S-Plus)Add Account
- from the new tab, clickConnect to Ledger
, then give it an alias, then clickAdd to wallet and close
namada
CLI, transfer tokens to your initial account and your Ledger (for testing both Bond tx from a local wallet account as well as from Ledger, as both of these are updated in this PR)Testing Bond tx
Staking
menu item (top menu)Stake
(there is an account drop down, but you don't need to change this if the account selected has funds, but you can as an option)Confirm
Bond
tx. Once this is approved, you should see a success message once it completesSCREENSHOTS
Ledger account loaded in extension
Ledger account loaded in interface