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: Add support for the Ledger Generic App #1408

Merged
merged 11 commits into from
Jul 29, 2024
Merged

Conversation

bee344
Copy link
Member

@bee344 bee344 commented Jul 23, 2024

It adds signsWithMetadata to support the new Polkadot Generic App. It also adds the support to send the signedTransaction back from the extension to the dApp.

Now the option to upload the metadata to the extension also uploads the rawMetadata as a HexString, that's necessary for CheckMetadataHash. The button also remains disabled until the rawMetadata is retrieved from the endpoint, in order to make sure the upload is done correctly.

This replaces #1397 .

Needs to be ran alongside #10778, because that has the support to uploading the metadata to the extension.

EDIT:

  • Added an error when trying to sign with a Ledger without the metadata present in the extension.
  • Updated the import process, because it broke with the Generic App and you could no longer import Ledger accounts other than Polkadot's.

@bee344 bee344 changed the title feat: Add support for the Ledger Generic App [WIP] feat: Add support for the Ledger Generic App Jul 24, 2024
@bee344 bee344 marked this pull request as ready for review July 24, 2024 13:28
setError: (value: string | null) => void;
}

function getMetadataProof (chain: Chain, payload: SignerPayloadJSON) {
const m = chain.definition.rawMetadata || '0x00';
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rework this little portion, when no metadata is present there should be some sort of error that is thrown, and logged on the screen.

@@ -102,7 +106,7 @@ export default function useLedger (genesis?: string | null, accountIndex = 0, ad
setError(null);
setWarning(null);

ledger.getAddress(false, accountIndex, addressOffset)
ledger.getAddress(chainInfo?.ss58Format || 0, false, accountIndex, addressOffset)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can do an assert here - We just want to make sure the ss58 is always for certain what is present in the ss58 format and can't resolve to 0

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

There are 2 little nits I have - But I also think we should have some way to warn about no metadata being present when users are using their ledger account

@TarikGul
Copy link
Member

Also we should consider what the workflow is also going to look like for the migration app.

@bee344 bee344 requested a review from TarikGul July 25, 2024 16:41
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

LGTM, great job - this will be followed up with support for the Migration App.

@bee344
Copy link
Member Author

bee344 commented Jul 26, 2024

@TarikGul Currently you have to add a new account for every chain, even though the derivation paths are the same, so it's kind of annoying. We could eventually add a "Allow use on any network" option like when creating via mnemonic.

@bee344 bee344 merged commit d2f1640 into master Jul 29, 2024
3 checks passed
@bee344 bee344 deleted the anp-ledger-generic-app branch July 29, 2024 12:03
@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jul 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants