Zondax / ledger-polkadot

Polkadot app for Ledger Nano S and X
Apache License 2.0
67 stars 32 forks source link

Staking calls not nested supported & not working with Proxies. #184

Closed rossbulat closed 1 year ago

rossbulat commented 1 year ago

There are some calls in Polkadot Staking Dashboard that cannot be made with a Ledger device because of their lack of nesting support (all proxied calls are nested in proxy.proxy).

Would it be possible to give nesting support to the following calls?:

Staking:

Nomination Pools:

Fast Unstake:

These would make proxy support feature complete with Ledger on the staking dashboard. Thank you.

:link: zboto Link

carlosala commented 1 year ago

Adding utility.batch to nesting is something that we want to avoid. Nesting batches of transactions might create a blob that's too big for the ledger. For the others, we'll take it into account in future versions. Thanks for the report!

rossbulat commented 1 year ago

Hi @carlosala, understood re utility.batch. We could introduce a new UI that, when a Ledger account is connected, forces users to sign separate txs in a step by step fashion, where batch txs are used.

Will this issue become less problematic as Ledger devices are shipped with better specs?

carlosala commented 1 year ago

You can use utility.batch in ledger. But you can't batch other batches.

rossbulat commented 1 year ago

You can use utility.batch in ledger. But you can't batch other batches.

I have indeed signed batch calls in my testing. My fear is that when using proxy.proxy to wrap a batch call, perhaps the Ledger device would not allow this.

carlosala commented 1 year ago

@rossbulat I'm afraid that's the problem. In the current architecture of the app, every time a call is added as a parameter (and not as the main extrinsic) it checks if that call is nesting supported. That applies for all batches and proxy, multisig, etc.

rossbulat commented 1 year ago

Would a batch([proxy.proxy(call1), proxy.proxy(call2)]) work? Instead of doing proxy.proxy(batch([call1, call2])).

(This is a note to myself to test this scenario).

If this works then we can work around batch not having nesting support. This assumes call1 and call2 have nesting support. proxy.proxy already has nesting support.

carlosala commented 1 year ago

that should make the trick, yes!

rossbulat commented 1 year ago

Awesome. Proxy support is being deployed to the staking dashboard later today. I have posted details on the proxy UI and Ledger support here on the Polkadot forum.

Ledger account proxies are recognised, but signing txs currently falls back to the delegator account. If nesting support can be rolled out to the calls mentioned in this issue, we can flip the switch and allow signing from a Ledger-imported proxy account :).

TLDR: all ready to go on the dashboard side.

carlosala commented 1 year ago

Super nice! It will be added in the next version👌🏻

carlosala commented 1 year ago

@rossbulat this has already been added. It'll be available in spec 9420. Thanks for the work!

rossbulat commented 1 year ago

Excellent stuff, can't wait to update and try out the proxy support. Thank you!