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: refactor, add ts and wasm #1

Closed
wants to merge 12 commits into from
Closed

Conversation

driemworks
Copy link
Contributor

@driemworks driemworks commented Nov 14, 2024

  • includes the wasm bindings and typescript wrapper for them (formerly etf-sdk-wasm and etf.js, respectively)
  • tightly couples the tle rust library, wasm bindings, and typescript wrapper, allowing for easy maintenance of updates to the API as the library evolves, as the ts piece of the library directly depends on the rust one.
  • tle.js becoms etf.js and we no longer publish the wasm-pack output to npm
    • The build script is modified to build the wasm and freshly install dependencies before transpiling to typescript
  • also includes several update to the tlock API to allow for greater flexibility:
    • introduces a StreamCipherProvider trait and AESGCMStreamCipherProvider implementation. This allows for the library to be unopinionated, allowing an implementor to specify the encryption mechanism and CSPRNG used. For example, we could implement tlock using age in the near future

Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

I know that many of the requested changes come inherited from the source repo. But we need to clean up.

  • Generally, we can't merge commented-out code to the main branch.
  • There are still mentions of ETF everywhere.
  • Some nits, like using capital letters to start docs comments, or add descriptions to todos

There are also more things to do but I reckon they should be done as separate issues, these are:

  • code formatting
  • use mocks in the tests
  • update the RPCs urls

tle/Cargo.toml Outdated Show resolved Hide resolved
tle/Cargo.toml Outdated Show resolved Hide resolved
tle/src/stream_ciphers.rs Outdated Show resolved Hide resolved
tle/src/tlock.rs Outdated Show resolved Hide resolved
tle/src/tlock.rs Outdated Show resolved Hide resolved
wasm/src/lib.rs Outdated Show resolved Hide resolved
wasm/src/lib.rs Outdated Show resolved Hide resolved
wasm/src/lib.rs Outdated Show resolved Hide resolved
wasm/src/lib.rs Outdated Show resolved Hide resolved
wasm/src/lib.rs Outdated Show resolved Hide resolved
@driemworks
Copy link
Contributor Author

I know that many of the requested changes come inherited from the source repo. But we need to clean up.

* Generally, we can't merge commented-out code to the main branch.

* There are still mentions of ETF everywhere.

* Some nits, like using capital letters to start docs comments, or add descriptions to todos

There are also more things to do but I reckon they should be done as separate issues, these are:

* code formatting

the same rustfmt.toml from the murmur crate is used here, except I shortened line lengths to 80 instead of 100, can change it back if you prefer but I like the shorter lines more.

* use mocks in the tests

What tests require mocks?

* update the RPCs urls
  • which rpcs? there aren't any here, I think this comment may have been for a diff repo?

tle/src/stream_ciphers.rs Outdated Show resolved Hide resolved
tle/src/stream_ciphers.rs Outdated Show resolved Hide resolved
@driemworks
Copy link
Contributor Author

* Generally, we can't merge commented-out code to the main branch.

we should implement a process that enforces this.

* There are still mentions of ETF everywhere.

The majority of these mentions are in the copied TS wrapper (etf.js). I removed references in the core/wasm libs but created a new issue to address etf.js (and migrate it to tle.js or whichever name we land on: #6

@juangirini
Copy link
Contributor

juangirini commented Nov 15, 2024

the same rustfmt.toml from the murmur crate is used here, except I shortened line lengths to 80 instead of 100, can change it back if you prefer but I like the shorter lines more.

I prefer 100, but is not a big deal if we are consistent across repos. The styling issues I saw were on JS actually

What tests require mocks?

This is what I meant #1 (comment)

  • which rpcs? there aren't any here, I think this comment may have been for a diff repo?

This is related to this comment #1 (comment)

@driemworks driemworks marked this pull request as draft November 18, 2024 14:12
@driemworks driemworks self-assigned this Nov 18, 2024
@driemworks driemworks marked this pull request as ready for review November 18, 2024 16:51
Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

I started reviewing it and got bored of code clean-up comments.
This needs some "search and replace" tle-> timelock, 'etf->timelock, ETF Network->Ideal Network`. File renaming, probably? Removing commented-out code.

/// The expected length of a nonce used with AES_GCM
const AES_GCM_NONCE_LEN: usize = 12;

/// Errors that mayb be encountered with using a stream cipher
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
/// Errors that mayb be encountered with using a stream cipher
/// Errors that may be encountered with using a stream cipher

ts/src/index.js Outdated
@@ -0,0 +1 @@
export * from './tle';
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
export * from './tle';
export * from './timelock';

"description": "a typescript sdk for timelock encryption/decryption with the Ideal Network",
"name": "@ideallabs/timelock.js",
"version": "0.0.1",
"description": "A typescript interface for timelock encryption",
"license": "GPL-3.0",
"repository": "https://github.com/ideal-lab5/tle",
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
"repository": "https://github.com/ideal-lab5/tle",
"repository": "https://github.com/ideal-lab5/timelock",

ts/src/tle.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this file be renamed to timelock?

ts/src/tle.js Outdated
export async function timelockEncrypt(encodedMessage, roundNumber, identityBuilder, beaconPublicKey, seed) {
await init();
// TODO: fine for now but should ultimately query the BABE pallet config instead
// https://github.com/ideal-lab5/tle/issues/7
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
// https://github.com/ideal-lab5/tle/issues/7
// https://github.com/ideal-lab5/timelock/issues/7

@@ -0,0 +1,40 @@
import { describe } from '@jest/globals';
import { timelockEncrypt, IdealNetworkIdentityHandler } from './tle';
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
import { timelockEncrypt, IdealNetworkIdentityHandler } from './tle';
import { timelockEncrypt, IdealNetworkIdentityHandler } from './timelock';

Comment on lines 21 to 39
// it('should timelock decrypt a message', async () => {
// const blockNumber = 1;
// const ciphertext = new Uint8Array(1);
// const signature = new Uint8Array(2);
// const result = await timelockDecrypt(ciphertext, signature);
// expect(result).toEqual({
// message: 'mocked-decrypted',
// sk: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
// })
// })
// it('should decrypt a message on demand if the user knows the secret', async () => {
// const secret = "shhh, it's a secret";
// const ciphertext = 'ciphertext'
// const result = await decrypt(ciphertext, secret);
// expect(result).toEqual({
// message: 'mocked-decrypted',
// sk: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
// })
// })
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
// it('should timelock decrypt a message', async () => {
// const blockNumber = 1;
// const ciphertext = new Uint8Array(1);
// const signature = new Uint8Array(2);
// const result = await timelockDecrypt(ciphertext, signature);
// expect(result).toEqual({
// message: 'mocked-decrypted',
// sk: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
// })
// })
// it('should decrypt a message on demand if the user knows the secret', async () => {
// const secret = "shhh, it's a secret";
// const ciphertext = 'ciphertext'
// const result = await decrypt(ciphertext, secret);
// expect(result).toEqual({
// message: 'mocked-decrypted',
// sk: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
// })
// })

@driemworks
Copy link
Contributor Author

Closing in place of simpler PR that only impacts the rust components, will address typescript wrapper in a new PR as well, tracked in #9 and #6

@driemworks driemworks closed this Nov 19, 2024
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.

2 participants