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

Full nacl implementation #2

Merged
merged 38 commits into from
Dec 28, 2020
Merged

Full nacl implementation #2

merged 38 commits into from
Dec 28, 2020

Conversation

pavlo-liapin
Copy link
Contributor

No description provided.

src/crypto-storage/crypto-storage.ts Show resolved Hide resolved
src/crypto-storage/crypto-storage.ts Show resolved Hide resolved
src/crypto-storage/crypto-storage.ts Outdated Show resolved Hide resolved
src/crypto-storage/crypto-storage.ts Show resolved Hide resolved
src/crypto-storage/crypto-storage.ts Show resolved Hide resolved
src/crypto-storage/crypto-storage.ts Outdated Show resolved Hide resolved
src/keyring/keyring.ts Outdated Show resolved Hide resolved
src/keyring/keyring.ts Outdated Show resolved Hide resolved
src/keyring/keyring.ts Outdated Show resolved Hide resolved
src/nacl/nacl.ts Show resolved Hide resolved
for (let i = 0; i < data.length; i++) {
encoded.push(String.fromCharCode(data[i]));
}
return encoded.join('');
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like decode/encode latin never used in glow

Copy link
Contributor Author

@pavlo-liapin pavlo-liapin Dec 18, 2020

Choose a reason for hiding this comment

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

Right. But I would keep it for now, there are more classes to move from Coffeescript Glow. If we still don't need them, I'll remove these methods

Copy link
Contributor

Choose a reason for hiding this comment

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

please don't forget

src/nacl/js-nacl-driver.ts Outdated Show resolved Hide resolved
const newTimeoutId = window.setTimeout(() => {
this.guestKeys.delete(guestTag);
this.guestKeyTimeouts.delete(guestTag);
}, config.RELAY_SESSION_TIMEOUT);
Copy link
Contributor

Choose a reason for hiding this comment

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

again reyking should not depend on relays

Copy link
Contributor Author

@pavlo-liapin pavlo-liapin Dec 18, 2020

Choose a reason for hiding this comment

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

Let's decide in the next PR about Mailbox and Relay classes, here I simply took the implementation for the old Glow. Maybe I'll move this stuff to Relay class, or maybe we don't need it at all

Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget

src/keyring/keyring.ts Outdated Show resolved Hide resolved
}
return encoded.join('');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think encode and decode latint here is outdated, please see https://stackoverflow.com/questions/8936984/uint8array-to-string-in-javascript

also we havbe similar methods in nacl one of them we need to remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextEncoder and TextDecoder don't work in jsdom environment, so all tests will fail. But let me think of this in the next PR, maybe I'll find a solution

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@lomchik lomchik self-requested a review December 24, 2020 12:42
@pavlo-liapin pavlo-liapin requested a review from lomchik December 28, 2020 11:18
if (!this.driverInstance) {
// fallback to the default JS driver
this.driverInstance = driver || new JsNaClDriver();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please throw an error if already was set. Developer should know if he doing something wrong

@pavlo-liapin pavlo-liapin merged commit 7ef7d23 into main Dec 28, 2020
@pavlo-liapin pavlo-liapin deleted the full-nacl-implementation branch December 28, 2020 11:32
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.

3 participants