Xmzheng/client sdk callformat lib #962

xmzheng · 2022-06-01T02:56:41Z

add encryption transport lib to web-ts. Add same test logic for derive_symmetric_key for golang, rust and typescript.

client-sdk/ts-web/rt/package.json

client-sdk/ts-web/package.json

pro-wh · 2022-06-01T16:38:10Z

client-sdk/ts-web/rt/src/callformat.ts

+        case types.CALLFORMAT_ENCRYPTED_X25519DEOXYSII:
+            if (cfg && cfg.publicKey) {
+                const privateKey = ed.utils.randomPrivateKey();
+                const publicKey = ed.curve25519.scalarMultBase(privateKey);


could we move this code for generating a keypair into the mrae file, to even out the level of abstraction here

reopening: you've instead moved key generation from callformat to the caller of encodeCall. we need to discuss this. but I need to review other reviewers' comments to see if there was anything else about this

client-sdk/ts-web/rt/src/callformat.ts

client-sdk/ts-web/rt/package.json

client-sdk/ts-web/rt/src/callformat.ts

client-sdk/ts-web/rt/src/mrae.ts

client-sdk/ts-web/rt/src/types.ts

pro-wh · 2022-06-01T16:50:54Z

oh also everybody's not been making "fmt" changes as separate commits, so could I ask you not to have that as a separate commit either

pro-wh · 2022-06-01T19:18:07Z

also can you link to the other languages' implementations?

xmzheng · 2022-06-01T19:31:45Z

@pro-wh thanks for your comments, I will update my diff.

this diff related to other implementations: oasisprotocol/oasis-core#4772

client-sdk/ts-web/rt/src/callformat.ts

client-sdk/ts-web/rt/src/mrae.ts

nhynes · 2022-06-04T15:43:46Z

client-sdk/ts-web/rt/src/mrae.ts

+    privateKey: Uint8Array,
+): Uint8Array {
+    const sharedKey = deriveSymmetricKey(publicKey, privateKey);
+    var aead = new deoxysii.AEAD(sharedKey);


This also has a non-trivial setup cost such that the cipher could (should) be cached. It's worse because the JS DeoxysII is not hw accelerated. For consistency with the other impls, though, I won't make a fuss about this. This is also not the lowest hanging fruit–just annoying.

how much setup cost is there in this setup? I see from the syntax that this creates an object, which must have some kind of state. is the setup separable from the lifetime of this state at all?

if it is, maybe we could expose something like nacl's 'box before' and 'box/open after' system https://www.npmjs.com/package/tweetnacl/v/1.0.3#naclboxbeforetheirpublickey-mysecretkey

this code doesn't have a concept of a session, so any repeated calls incur overhead of setting up the shared secret and initializing deoxysii state. if there were sessions, the deoxysii setup and x25519 could be saved

client-sdk/ts-web/rt/test/mrae.test.ts

pro-wh

thanks for the changes so far

client-sdk/ts-web/rt/src/mrae.ts

client-sdk/ts-web/rt/src/callformat.ts

client-sdk/ts-web/rt/package.json

pro-wh · 2022-06-21T19:09:56Z

client-sdk/ts-web/rt/src/callformat.ts

+    call: types.Call,
+    format: types.CallFormat,
+    config?: EncodeConfig,
+): Promise<[types.Call, MetaEncryptedX25519DeoxysII?]> {


comparing with this

oasis-sdk/client-sdk/go/callformat/callformat.go

Lines 31 to 32 in a0da10b

// It returns the encoded call and any metadata needed to successfully decode the result.

func EncodeCall(call *types.Call, cf types.CallFormat, cfg *EncodeConfig) (*types.Call, interface{}, error) {

, it looks like the intention is to allow metadata types other than the x25519deoxysii. maybe we should declare it as unknown, similar to the interface{} they use in the go version? what do you think?

Strictly speaking, you could use something like this to implement generic associated types. Go doesn't have that ability, which is why it's interface{}.

With the aim of not suggesting sweeping changes to the codebase, unknown is fine since at least it won't be a breaking change when other metadatas are added. It'll just be inconvenient to inspect the metadata.

I guess I'll check on that sample later today 🙊

ugh we used to do this thing in Go where there would be an overarching struct with a named field for each variant.

ok I loaded up the sample, but it's not clear how to go between associated Metadata.* and CallFormat.* type. (seems incidental and slightly abusive that CallFormat.Plain = Metadata.Plain.)

another answer would be to have multiple typescript declaration overloads of such an encode function. as nhynes says though, feel free not to and use unknown instead

I think it's fine that these types come out of encode relatively opaque. users will only pass them verbatim to submit and decode, respectively, neither of which internally could take advantage more type info

client-sdk/ts-web/rt/src/callformat.ts

pro-wh · 2022-06-21T19:46:22Z

client-sdk/ts-web/rt/src/callformat.ts

+        case types.CALLFORMAT_ENCRYPTED_X25519DEOXYSII:
+            if (cfg && cfg.publicKey) {
+                const privateKey = ed.utils.randomPrivateKey();
+                const publicKey = ed.curve25519.scalarMultBase(privateKey);


reopening: you've instead moved key generation from callformat to the caller of encodeCall. we need to discuss this. but I need to review other reviewers' comments to see if there was anything else about this

pro-wh · 2022-06-21T19:49:48Z

client-sdk/ts-web/rt/src/callformat.ts

+import * as types from './types';
+import * as mrae from './mrae';
+
+var deoxysii = require('deoxysii');


643221f#diff-3253cbee91843f44bfc8dd19a126548d016d25c34bf4ce34f3b8f1f93dd9b47fR3-R4

the sad answer is still to do that and have typescript ignore that error

client-sdk/ts-web/rt/src/callformat.ts

client-sdk/ts-web/rt/src/mrae.ts

client-sdk/ts-web/package.json

nhynes · 2022-06-27T04:10:59Z

client-sdk/ts-web/rt/src/callformat.ts

+    call: types.Call,
+    format: types.CallFormat,
+    config?: EncodeConfig,
+): Promise<[types.Call, MetaEncryptedX25519DeoxysII?]> {


Strictly speaking, you could use something like this to implement generic associated types. Go doesn't have that ability, which is why it's interface{}.

With the aim of not suggesting sweeping changes to the codebase, unknown is fine since at least it won't be a breaking change when other metadatas are added. It'll just be inconvenient to inspect the metadata.

client-sdk/ts-web/rt/src/mrae.ts

pro-wh

some philosophical stuff

pro-wh · 2022-06-27T18:09:52Z

client-sdk/ts-web/rt/src/callformat.ts

+        return oasis.misc.fromCBOR(pt) as types.CallResult;
+    } else if (result.fail) {
+        throw new Error(
+            `callformat: failed call: module :${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,


Suggested change

`callformat: failed call: module :${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,

`callformat: failed call: module: ${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,

other than that, is this right for us to throw in a "decode" function? like wouldn't be more fitting of the function's objectives to return a CallResult that indicates a runtime failure? or is result.fail not the kind of error I'm thinking of?

edit: ugh the go side does this. I hope they have a good reason......................................

pro-wh · 2022-06-27T18:12:01Z

client-sdk/ts-web/rt/src/callformat.ts

+        );
+    }
+
+    throw new Error(`callformat: unexpected result: ${result.ok}`);


I feel like maybe we'd want to see the whole result object, so that we could see if it were completely the wrong type or something. purely speculative, as we don't any experience of this happening. but would that be okay with you?

but then on the other hand, we'd probably only see "[object Object]" 🤷

what I usually do is attach the result/cause to what I throw. something like this can be used to keep TS happy.

source 👍

pro-wh · 2022-06-27T19:00:37Z

client-sdk/ts-web/rt/src/callformat.ts

+                throw new Error('callformat: runtime call data public key not set');
+            }
+            const rawCall = oasis.misc.toCBOR(call);
+            const sealedCall = mrae.boxSeal(nonce, rawCall, null, config.publicKey.key, sk);


philosophical question about the layers of abstraction here:

this callformat asks for x25519 deoxysii

the primitive we've currently selected to occupy the mrae ts module is x25519 deoxysii

we use the mrae module here

but the mrae module is named agnostically. does that mean we're not sure it'll always be x25519 deoxysii? if we change out the selected crypto primitive, how would we do it? change it directly and fix this callsite? add the new primitive in a new module instead? redefine the callformat as well?

or is the binding of x25519 deoxysii to the oasis projects' notion of mrae a permanent thing?

on the go side

oasis-sdk/client-sdk/go/callformat/callformat.go

Line 11 in a0da10b

mraeDeoxysii "github.com/oasisprotocol/oasis-core/go/common/crypto/mrae/deoxysii"

, the module is named deoxysii 🤷

client-sdk/ts-web/rt/src/callformat.ts

pro-wh · 2022-06-27T19:18:55Z

client-sdk/ts-web/rt/src/mrae.ts

+    privateKey: Uint8Array,
+): Uint8Array {
+    const sharedKey = deriveSymmetricKey(publicKey, privateKey);
+    var aead = new deoxysii.AEAD(sharedKey);


how much setup cost is there in this setup? I see from the syntax that this creates an object, which must have some kind of state. is the setup separable from the lifetime of this state at all?

if it is, maybe we could expose something like nacl's 'box before' and 'box/open after' system https://www.npmjs.com/package/tweetnacl/v/1.0.3#naclboxbeforetheirpublickey-mysecretkey

codecov · 2022-07-06T16:12:51Z

Codecov Report

Merging #962 (ed84b41) into main (b4efbe8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #962   +/-   ##
=======================================
  Coverage   67.96%   67.96%           
=======================================
  Files         128      128           
  Lines       11017    11017           
=======================================
  Hits         7488     7488           
  Misses       3497     3497           
  Partials       32       32

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4efbe8...ed84b41. Read the comment docs.

pro-wh · 2022-07-06T17:14:10Z

client-sdk/ts-web/rt/test/callformat.test.ts

+      const pairs = nacl.box.keyPair()
+      const publicKey = pairs.publicKey
+      const rawCall: oasisRT.types.Call = {
+	format: oasisRT.transaction.CALLFORMAT_ENCRYPTED_X25519DEOXYSII,


some tabs here, could you convert to spaces

pro-wh · 2022-07-06T17:22:37Z

client-sdk/ts-web/rt/src/callformat.ts

+ * encodeCallWithNonceAndKeys encodes a call based on its configured call format.
+ * It returns the encoded call and any metadata needed to successfully decode the result.
+ */
+export function encodeCallWithNonceAndKeys(


in some cases, we're making cryptography-related operations unnecessarily async, e.g. hashing a public key into an address. this is so that we could more easily change implementations in the future, if a more preferred library comes out exposing an async interface. or if the primitives we use later become available in the async web crypto interface and we could use that.

on the other hand, in other places, such as converting between binary and bech32 addresses, it's a synchronous interface. I think these places are the ones where it's a more niche kind of operation, and there's a single well established library for doing it, and that library does it synchronously.

I think this particular case is closer to the first scenario. but this is a late coming comment, and I understand we're interested in merging this right away to support some ongoing work elsewhere, so eh

I am going to change the signature.

pro-wh

thanks for the changes, let's get this in so people can start using it

xmzheng · 2022-07-06T18:30:30Z

client-sdk/ts-web/rt/src/callformat.ts

+): Promise<[types.Call, unknown]> {
+    const nonce = randomBytes(deoxysii.NonceSize);
+    const keyPair = nacl.box.keyPair();
+    return await encodeCallWithNonceAndKeys(


do I need to use await here? I think I can directly return the Promise, right?

you're right about that. stylistically I've preferred to await inside because it makes it easier to move code around

xmzheng requested a review from nhynes June 1, 2022 02:56

xmzheng requested review from kostko, pro-wh and ptrus as code owners June 1, 2022 02:56

pro-wh reviewed Jun 1, 2022

View reviewed changes

nhynes reviewed Jun 4, 2022

View reviewed changes

pro-wh reviewed Jun 21, 2022

View reviewed changes

nhynes reviewed Jun 27, 2022

View reviewed changes

xmzheng requested a review from pro-wh June 27, 2022 16:29

pro-wh reviewed Jun 27, 2022

View reviewed changes

xmzheng requested review from nhynes and pro-wh July 2, 2022 02:56

nhynes approved these changes Jul 2, 2022

View reviewed changes

James Zheng added 8 commits July 6, 2022 09:06

init commit

6485018

add unittest for mrae functions

8af546d

fmt

084158f

use tweetnacl and address other comments

07cb480

address more comments

18956b1

use arrayBuffer and ignore declaration error

f3567d5

add one more unittest and address more comments

8fb3f0a

add the callformat unit test

6a271c9

xmzheng force-pushed the xmzheng/client-sdk-callformat-lib branch from d6744fe to 6a271c9 Compare July 6, 2022 16:08

small format changes

59d8beb

fix npm install issues

8ca5ecb

pro-wh reviewed Jul 6, 2022

View reviewed changes

pro-wh approved these changes Jul 6, 2022

View reviewed changes

change encodeCall/decodeCall signatures to be async

bc5d02e

xmzheng commented Jul 6, 2022

View reviewed changes

fix the lint issue on test code

ed84b41

xmzheng merged commit 84c1789 into main Jul 6, 2022

xmzheng deleted the xmzheng/client-sdk-callformat-lib branch July 6, 2022 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xmzheng/client sdk callformat lib #962

Xmzheng/client sdk callformat lib #962

xmzheng commented Jun 1, 2022

pro-wh Jun 1, 2022

pro-wh Jun 21, 2022

pro-wh commented Jun 1, 2022

pro-wh commented Jun 1, 2022

xmzheng commented Jun 1, 2022

nhynes Jun 4, 2022

pro-wh Jun 27, 2022

nhynes Jun 28, 2022

pro-wh left a comment

pro-wh Jun 21, 2022

nhynes Jun 27, 2022

pro-wh Jun 27, 2022

pro-wh Jun 27, 2022

pro-wh Jun 21, 2022

pro-wh Jun 21, 2022

nhynes Jun 27, 2022

pro-wh left a comment

pro-wh Jun 27, 2022

pro-wh Jun 27, 2022

pro-wh Jun 27, 2022

nhynes Jun 28, 2022

pro-wh Jun 28, 2022

pro-wh Jun 27, 2022

pro-wh Jun 27, 2022

codecov bot commented Jul 6, 2022 •

edited

Loading

pro-wh Jul 6, 2022

pro-wh Jul 6, 2022

xmzheng Jul 6, 2022

pro-wh left a comment

xmzheng Jul 6, 2022

pro-wh Jul 6, 2022

	// It returns the encoded call and any metadata needed to successfully decode the result.
	func EncodeCall(call types.Call, cf types.CallFormat, cfg EncodeConfig) (*types.Call, interface{}, error) {

	`callformat: failed call: module :${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,
	`callformat: failed call: module: ${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,

Xmzheng/client sdk callformat lib #962

Xmzheng/client sdk callformat lib #962

Conversation

xmzheng commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pro-wh commented Jun 1, 2022

pro-wh commented Jun 1, 2022

xmzheng commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pro-wh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pro-wh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pro-wh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2022 •

edited

Loading