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

fix: corrupted paper-wallet backup #4

Merged
merged 4 commits into from
Jan 17, 2022
Merged

Conversation

OlegMakarenko
Copy link
Contributor

@OlegMakarenko OlegMakarenko commented Jan 6, 2022

What the current behaviour is
The paper wallet lib uses 2 encoded PDF docs. The first one contains Mnemonic page, the second one -Account page. The output of Paper Wallet lib is a single PDF doc which always contains 1 Mnemonic page and Account pages depending on the number of accountInfos passed to the constructor. To do it the paper wallet lib uses the "Mnemonic" PDF as a base and copies pages from "Account" PDF.

Why this is an issue.
There is an issue with the copyPages() method form "pdf-lib" which is used to copy Account page from one PDF doc to another. Copied page is corrupted.

image

What's the fix
This PR changes the way how the Account page is cloning. Instead of coping Account page from separate (foreign) PDF doc, now Mnemonic and Account pages are already in a single doc. It lets to avoid using a copyPages() method form "pdf-lib" which introduces an issue.

@0x6861746366574
Copy link
Contributor

@yilmazbahadir and @AnthonyLaw to review. Please make sure to fix any spelling errors throughout the commit.

It's not clear what the issue was originally and how this fixes it. Could you extend the original issue following the format seen here?

As a rule of thumb, you want to describe these three things:

  • What the current behaviour is.
  • Why this is an issue.
  • How you changed this behaviour.

@AnthonyLaw
Copy link
Member

The code looks good, but 2 problems for me.

  1. I have a problem with npm i in the packages/paper-wallets folder.
  2. how to run the code?

@yilmazbahadir
Copy link
Contributor

The code looks good, but 2 problems for me.

  1. I have a problem with npm i in the packages/paper-wallets folder.
  2. how to run the code?

I think this addresses the same question with a suggestion => #5 (comment)

Copy link
Contributor

@yilmazbahadir yilmazbahadir left a comment

Choose a reason for hiding this comment

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

Can you run lint:fix in the whole project before you merge this one? (after the second PR merged ofc)

@@ -98,17 +97,39 @@ class SymbolPaperWallet {
* Exports as a PDF Uin8Array
*/
async toPdf(): Promise<Uint8Array> {
// Load teplate pdf document. It consists of 2 pages - mnemonic and account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Load teplate pdf document. It consists of 2 pages - mnemonic and account.
// Load template pdf document. It consists of 2 pages - mnemonic and account.

@OlegMakarenko
Copy link
Contributor Author

@0x6861746366574 description updated.

@OlegMakarenko
Copy link
Contributor Author

@AnthonyLaw

I have a problem with npm i in the packages/paper-wallets folder.

What's the issue? Can you please provide a console log.

how to run the code?

You can build it locally and follow these instructions:
https://github.com/symbol/wallets-lib/tree/main/packages/paper-wallets

@OlegMakarenko OlegMakarenko changed the base branch from main to dev January 7, 2022 20:50
@AnthonyLaw
Copy link
Member

@AnthonyLaw

I have a problem with npm i in the packages/paper-wallets folder.

What's the issue? Can you please provide a console log.

how to run the code?

You can build it locally and follow these instructions: https://github.com/symbol/wallets-lib/tree/main/packages/paper-wallets

I think the problem caused by the M1 chip.
image

@OlegMakarenko
Copy link
Contributor Author

@AnthonyLaw can you please open a new issue regarding this?

@0x6861746366574 0x6861746366574 changed the title fix: pdf page clonning fix: corrupted paper-wallet backup Jan 11, 2022
@0x6861746366574 0x6861746366574 deleted the fix-pdf-page-clonning branch January 11, 2022 02:14
@0x6861746366574 0x6861746366574 restored the fix-pdf-page-clonning branch January 11, 2022 02:14
@0x6861746366574
Copy link
Contributor

Oops. Tried to rename the branch and forgot PR's are very particular. 😁

Copy link
Contributor

@yilmazbahadir yilmazbahadir left a comment

Choose a reason for hiding this comment

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

Deleting package-lock.json?

@OlegMakarenko
Copy link
Contributor Author

OlegMakarenko commented Jan 13, 2022

Deleting package-lock.json?

@yilmazbahadir yes. This is the old one from packages/paper-wallets/.
We still have package-lock.json in the project root folder

@yilmazbahadir
Copy link
Contributor

yilmazbahadir commented Jan 13, 2022

Deleting package-lock.json?

@yilmazbahadir yes. This is the old one from packages/paper-wallets/. We still have package-lock.json in the project root folder :)

Doesn't seem right to me if we want to turn this into a monorepo, I think every package should have its own package.json otherwise it wouldn't be a package, right? I think we haven't decided the tool for the monorepo yet, but it could be lerna or bazel or what the team has decided to use in symbol monorepo.

Just to verify @0x6861746366574, do we want to bundle the dependencies together and release them as a single package(wallets-lib) or do we want to turn this into a monorepo and release it as separate packages in the near future? Stuffing sounds like it is the former(single package) from your discord message just wanted to verify before we proceed.

@0x6861746366574
Copy link
Contributor

Deleting package-lock.json?

@yilmazbahadir yes. This is the old one from packages/paper-wallets/. We still have package-lock.json in the project root folder :)

Doesn't seem right to me if we want to turn this into a monorepo, I think every package should have its own package.json otherwise it wouldn't be a package, right? I think we haven't decided the tool for the monorepo yet, but it could be lerna or bazel or what the team has decided to use in symbol monorepo.

Just to verify @0x6861746366574, do we want to bundle the dependencies together and release them as a single package(wallets-lib) or do we want to turn this into a monorepo and release it as separate packages in the near future? Stuffing sounds like it is the former(single package) from your discord message just wanted to verify before we proceed.

Correct, we want to package them as a single library in the interim.
Once we do the rewrite, the QR library and HD wallet will be deprecated (as this can be handled by the SDK). The address book should simply be a module of the headless client.

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.

4 participants