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

677 replace eth crypto #678

Merged
merged 4 commits into from Jan 7, 2020
Merged

677 replace eth crypto #678

merged 4 commits into from Jan 7, 2020

Conversation

@pedrouid
Copy link
Member

pedrouid commented Jan 6, 2020

Fix #677

pedrouid added 2 commits Jan 3, 2020
…o 677-replace-eth-crypto
@todo-tracker

This comment has been minimized.

Copy link

todo-tracker bot commented Jan 6, 2020

Hey, pedrouid

We noticed you made changes to a file with a TODO on it.
These are set to make sure potential Technical Debt doesn't get forgotten.
While you're here take a shot at turning a listed TODO into a TODONE!

Id Name File Priority
77 how do we know if we're in prod mode or not? ops/test-integration.sh Normal

button

@LayneHaber LayneHaber self-requested a review Jan 7, 2020
Copy link
Contributor

LayneHaber left a comment

nice job!

@LayneHaber

This comment has been minimized.

Copy link
Contributor

LayneHaber commented Jan 7, 2020

still need to test in react native though, couldn't get to it today

mac: string;
};

export function removeTrailing0x(str: string): string {

This comment has been minimized.

Copy link
@bohendo

bohendo Jan 7, 2020

Member

Trailing0x? Think you mean Leading0x

return str;
}

export function addTrailing0x(str: string): string {

This comment has been minimized.

Copy link
@bohendo

bohendo Jan 7, 2020

Member

This function shouldn't be exported since it's never used outside this module

const cipher = EthCrypto.parse(encryptedPreImage);

const preImage = await EthCrypto.decryptWithPrivateKey(privateKey, cipher);
Comment on lines 834 to 836

This comment has been minimized.

Copy link
@bohendo

bohendo Jan 7, 2020

Member

Why generate this intermediate cipher var? If it's never used, then we should hide it w/in the EthCrypto module & simplify the decryptWithPrivateKey function sig accordingly

import { HashZero, Zero } from "ethers/constants";
import { fromExtendedKey } from "ethers/utils/hdnode";

import * as EthCrypto from "../eth-crypto";

This comment has been minimized.

Copy link
@bohendo

bohendo Jan 7, 2020

Member

Which functions from the crypto module do we need? Specifying im/exports explicitly draws attention to the interface between this module and the rest of the codebase which encourages us to keep it as simple as possible which makes the code easier to maintain.

const encryptedPreImage = EthCrypto.stringify(encryptedPreImageCipher);
await this.connext.setRecipientAndEncryptedPreImageForLinkedTransfer(
recipient,
encryptedPreImage,
Comment on lines 93 to 96

This comment has been minimized.

Copy link
@bohendo

bohendo Jan 7, 2020

Member

The encryptedPreImage var isn't ever used here, it should be hidden w/in the crypto module

@bohendo bohendo merged commit ed879d1 into staging Jan 7, 2020
9 checks passed
9 checks passed
build
Details
test-cf
Details
test-node
Details
test-bot
Details
test-cf
Details
test-integration
Details
test-bot
Details
test-bot-farm
Details
test-daicard
Details
@bohendo bohendo deleted the 677-replace-eth-crypto branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.