Add codec for BNS transactions #56

ethanfrey · 2018-06-12T17:34:29Z

Build protobuf definitions from bov go source repos
Create postable transaction bytes for all types
Create signable transaction bytes for all types
Calculate the transaction id (tx_hash)
Parse existing transactions from the blockchain
Test self-compatibility (encode/decode)
Test against reference testdata exported from bov binary

ethanfrey · 2018-06-12T18:15:55Z

Hmmm... I was trying to implement parsing and realized this is impossible as specified.

When we create an @iov/types Transaction object, it contains PublicKeyBundle as identifiers to allow us to encode it in a chain-dependent format:

export interface BaseTx {
  readonly chainId: ChainID;
  readonly fee: FungibleToken;
  readonly signer: PublicKeyBundle;
  readonly ttl?: TTLBytes;
}

export interface SendTx extends BaseTx {
  readonly kind: TransactionKind.SEND;
  readonly amount: FungibleToken;
  readonly recipient: PublicKeyBundle;
}

However, when we parse a transaction, we would only be able to receive the chain-dependent address of the recipient, and not the public key from which it was derived. Since Transaction is designed for the API interface we shouldn't force them to encode this if they only have the public key.

I think this API change should go into another PR, but one thought was to change it to:

  readonly recipient: PublicKeyBundle | AddressBytes;

This would allow parsing code to return a proper tx and we can differentiate between the two in runtime (one a Uint8Array, the other an object). I'm not sure if this is going to seep out and lead to lots of uncertainty in the type system, or if there are other negative effects.

Thoughts?

ethanfrey · 2018-06-12T19:33:39Z

Before you think I am a coding monster... It's called codegen ;)

wc -l src/codec.*
    2295 src/codec.d.ts
    5603 src/codec.js
    7898 total

That's all from protobufjs... only 2k from me

webmaster128 · 2018-06-13T05:27:54Z

I think instead of reusing the type, it would be better to create two different types. If I understood this correctly, we would have:

Website to extension via web4: SigningRequest, SendSigningRequest
Blockchain to extension directly: BaseTx, SendTx

webmaster128 · 2018-06-13T05:32:41Z

packages/bns-codec/tests/testdata.ts

+  return out;
+}
+
+export function fromHex(hexstring: string): Uint8Array {


Please reuse #57 after merge

ethanfrey · 2018-06-13T15:21:31Z

I think instead of reusing the type, it would be better to create two different types. If I understood this correctly, we would have:

Website to extension via web4: SigningRequest, SendSigningRequest
Blockchain to extension directly: BaseTx, SendTx

Actually I based the entire design that we always get a public key for the recipient from the BNS. As I am working, I see three flaws.

The BNS may no longer serve public keys but pre-digested addresses (or per-chain public keys)
I may get a chain-specific address from some other (non-web4) source, like a bitcoin address from another wallet.
I may try to parse a prepared tx, either from the blockchain, or when preparing multisig.

I think using Addresses instead of PublicKeyBundle for recipient is actually better.
And I probably should standardize for escrow src and dest as well. (change in go code)

Whether we support PublicKeyBundle at all for recipients depends on outcome of BNS web4 key lookup design sessions.

Isabello · 2018-06-13T16:38:16Z

The BNS may no longer serve public keys but pre-digested addresses (or per-chain public keys)
I may get a chain-specific address from some other (non-web4) source, like a bitcoin address from another wallet.
I may try to parse a prepared tx, either from the blockchain, or when preparing multisig.

The BNS will serve the following:

address:pubkey pairs for the BNS specific coin path, that can be safely reused on any compatible chain and any chain implementing bip44
Allow for additional address:pubkey pairs to be added to the NFT holding the human readable name.

I think using Addresses instead of PublicKeyBundle for recipient is actually better.

Addresses should be used for recipient, as nice as publicKey . In the case of escrow, we can use any address we want, all that needs to be the same between transactions is the publickey and the associated signature

webmaster128

First set of comments

webmaster128 · 2018-06-14T10:11:31Z

packages/bns-codec/package.json

+    "test": "yarn build && yarn test-node",
+    "prebuild": "yarn lint && yarn format",
+    "build": "tsc && cp ./src/codec.* ./build/src",
+    "findProto": "find $GOPATH/src/github.com/confio/weave $GOPATH/src/github.com/iov-one/bcp-demo -name '*.proto' -not -path '*/vendor/*' -not -path '*/examples/*'",


We use dash separated yarn subcommands usually

thanks. i knew something looked funny

webmaster128 · 2018-06-14T10:38:33Z

packages/bns-codec/src/util.ts

+
+const map = Array.prototype.map;
+const toNums = (str: string) => map.call(str, (x: string) => x.charCodeAt(0));
+export const stringToArray = (str: string) => Uint8Array.from(toNums(str));


This method is supposed to be called "asciiEncode", right?

Yes, it should have ascii in the name.
decodeAscii I believe.... a string has an encoding. bytes are just bytes.
at least in python, I do something like "deadbeef".decode('hex').encode('base64')

A string has no encoding. A string is usually a list of unicode codepoints. Now there are different possibilities to encode that string in a binary representation (ascii, utf-8, utf-16, …).

Python 3 (using Python 3 is important here) example:

>>> "abc" 'abc' >>> "abc".encode('ascii') b'abc' >>> "abc".encode('utf16') b'\xff\xfea\x00b\x00c\x00' >>> "abc".decode() Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'str' object has no attribute 'decode'

Python 2 has no proper differentiation between a string and its binary representation. This is why your example works in Python 2 but not Python 3.

webmaster128 · 2018-06-14T10:42:40Z

packages/bns-codec/src/util.ts

+};
+
+const map = Array.prototype.map;
+const toNums = (str: string) => map.call(str, (x: string) => x.charCodeAt(0));


You can avoid the use of that extra map:

const toNums = (str: string) => str.split('').map((x: string) => x.charCodeAt(0));

webmaster128 · 2018-06-14T10:47:47Z

packages/bns-codec/src/util.ts

+
+// append chainID and nonce to the raw tx bytes to prepare for signing
+export const appendSignBytes = (bz: Uint8Array, chainID: ChainID, nonce: Nonce) =>
+  Uint8Array.from([...bz, ...stringToArray(chainID), ...nonce.toBytesBE()]) as SignableBytes;


let's discuss this. please open a new issue in confio/weave.

The codec demands compatibility with the blockchain implementation, but you do have a point that the blockchain implementation may need improvement. I think this should not block the PR, but we should discuss a better sign bytes format in weave, then update across the board.

chainIDlength prefix is good idea.
format version nice for future compatibility

webmaster128 · 2018-06-14T10:53:42Z

packages/bns-codec/src/util.ts

+// tendermint hash (will be) first 20 bytes of sha256
+// probably only works after 0.21, but no need to import ripemd160 now
+export const tendermintHash = (data: Uint8Array) =>
+  Sha256.digest(data).then((bz: Uint8Array) => bz.slice(0, 20));


We can make Sha256.digest synchronous if the asynchronous interface causes pain for the caller.

sha.js is synchronous and in nodejs we could alternatively use crypto.createHash('sha256').update('alice', 'utf8').digest(), which is synchronous as well

if there is a good reason, we can keep it async.
my main thought was that encoding is a synchronous action (no IO)
with async/await it is not hard to chain promises or async calls, and it isn't really bad.
however, it forced me to change the API.

not a super strong objection, I just want to see if it causes more pain to have sync sha256, or async encoding

we can leave as async and then decide if we want to change in another PR.

👍 just wanted to offer to change it. I see no need to have sha256 asynchronous at the moment

webmaster128 · 2018-06-14T11:04:00Z

packages/bns-codec/tests/encode.spec.ts

+
+const { fromHex, toHex } = Encoding;
+
+describe("Control", () => {


This test is obsolete now

👍 thanks for that code

webmaster128

decodeAscii -> encodeAscii or encodeAsAscii

webmaster128 · 2018-06-14T13:12:39Z

packages/bns-codec/src/util.ts

@@ -30,6 +30,14 @@ export const appendSignBytes = (bz: Uint8Array, chainID: ChainID, nonce: Nonce)
 export const tendermintHash = (data: Uint8Array) =>
  Sha256.digest(data).then((bz: Uint8Array) => bz.slice(0, 20));

-// TODO: verify prefix, make const
+export const HashId = decodeAscii("hash/sha256/");


This is not a type -> lowecase hashId

ethanfrey · 2018-06-14T17:04:59Z

I think I am ready now. Tested everything I could think of.
Please merge if no more complaints, or add more review comments.

will-iov · 2018-06-14T19:35:40Z

packages/bns-codec/jasmine-testrunner.js

+    verbosity: 4,        // (0|false)|1|2|(3|true)|4
+    listStyle: 'indent', // "flat"|"indent"
+    activity: true,
+    emoji: true,


This is giving you a 👊 when all tests passed ;)

will-iov · 2018-06-14T19:49:12Z

packages/bns-codec/src/decode.ts

+  recipient: ensure(msg.dest, "recipient") as AddressBytes,
+  amount: decodeToken(ensure(msg.amount)),
+  memo: msg.memo || undefined,
+  ...base,


Does it definitely make more sense to put this last rather than first? If base has some extra properties somehow they can override whatever came before.

will-iov · 2018-06-14T19:53:55Z

packages/bns-codec/src/encode.ts

+  });
+};
+
+export const buildMsg = (tx: UnsignedTransaction): Promise<codec.app.ITx> => {


How do you feel about marking all Promise-returning functions as async whether or not they make use of await?

i dunna, i figured unnecessary.
what does @webmaster128 think?
if you both like it, i'll adopt.

It should be unnecessary runtime overhead. Changing a function from synchronous to async is a compatible change when you return a Promise. So you can switch to async later.

will-iov · 2018-06-14T19:58:30Z

packages/bns-codec/src/index.ts

+  public static async bytesToSign(tx: UnsignedTransaction, nonce: Nonce): Promise<SignableBytes> {
+    // we encode it without any signatures
+    const built = await buildUnsignedTx(tx);
+    const bz = codec.app.Tx.encode(built).finish();


What does bz stand for?

bytes.
i mean, bs is also short for bytes, but doens't sound so good (B.S.)

in all my years in software development, I never came across any of those two abbreviations. So I'd prefer to use a proper word.

you clearly never worked at tendermint ;)
but sure, I can use a real word....

will-iov · 2018-06-14T20:02:42Z

packages/bns-codec/src/index.ts

+}
+
+// we need to create a const to properly type-check the export...
+// export const BNSCodec: TxCodec = Codec;


What's wrong with export class BNSCodec implements TxCodec { ... }? (Or even export default.)

It doesn't implement it somehow, as implements implies an instance of the class implements it, not the static class object....

If you can make it work, just fighting tsc sometimes.
Also, it does no longer match with the async changes.

will-iov · 2018-06-14T20:07:15Z

packages/bns-codec/src/types.ts

+
+export const decodeSignature = (signature: codec.crypto.ISignature): SignatureBytes => {
+  if (signature.ed25519) {
+    return signature.ed25519 as SignatureBytes;


There's an asymmetry here with keys: should we maybe be returning an object with algo/data here too?

I guess so, not sure why it was defined differently. But it has been for a while.
I would do that change as another PR, but good point.

will-iov · 2018-06-14T20:14:19Z

packages/bns-codec/src/util.ts

+export const hashIdentifier = async (data: Uint8Array) =>
+  Uint8Array.from([...HashId, ...(await Sha256.digest(data))]);
+
+// typescript forces us to return number on reduce, so we count how many elements match


typescript forces us to return number on reduce

Really?!

Try it, if you can get it to reduce an Uint8Array to a bool, please show me the code.
They should have a few more T generic types in their Uint8Array type definition....

will-iov · 2018-06-14T20:16:26Z

packages/bns-codec/src/util.ts

+// and make sure it is all
+export const arraysEqual = (a: Uint8Array, b: Uint8Array): boolean =>
+  a.length === b.length &&
+  a.reduce((acc: number, x: number, i: number) => (x === b[i] ? acc + 1 : acc), 0) === b.length;


How about

a.length === b.length && a.every((n: number, i: number): boolean => n === b[i])

hmm... did you run that one through tsc?

In any case, this method needs unittests

Tested the every implementation by Will, works. I think it is way easier to understand. Here are the tests:

it("arraysEqual works", () => { // simple equality expect(arraysEqual(new Uint8Array([]), new Uint8Array([]))).toEqual(true); expect(arraysEqual(new Uint8Array([0x11]), new Uint8Array([0x11]))).toEqual(true); expect(arraysEqual(new Uint8Array([0xaa, 0x77, 0x99]), new Uint8Array([0xaa, 0x77, 0x99]))).toEqual(true); // identity const array1 = new Uint8Array([]); const array2 = new Uint8Array([0x11]); const array3 = new Uint8Array([0xaa, 0x77, 0x99]); expect(arraysEqual(array1, array1)).toEqual(true); expect(arraysEqual(array2, array2)).toEqual(true); expect(arraysEqual(array3, array3)).toEqual(true); // unequal length expect(arraysEqual(new Uint8Array([]), new Uint8Array([0x11]))).toEqual(false); expect(arraysEqual(new Uint8Array([0xaa, 0xbb]), new Uint8Array([0xaa]))).toEqual(false); expect(arraysEqual(new Uint8Array([0xaa]), new Uint8Array([0xaa, 0xbb]))).toEqual(false); // unequal data (front, middle, end) expect(arraysEqual(new Uint8Array([0xaa, 0xbb, 0xcc]), new Uint8Array([0x00, 0xbb, 0xcc]))).toEqual(false); expect(arraysEqual(new Uint8Array([0xaa, 0xbb, 0xcc]), new Uint8Array([0xaa, 0x00, 0xcc]))).toEqual(false); expect(arraysEqual(new Uint8Array([0xaa, 0xbb, 0xcc]), new Uint8Array([0xaa, 0xbb, 0x00]))).toEqual(false); });

there are a few unit tests in codec.spec.ts, but i will add these tests and update to Will's implementation.

will-iov · 2018-06-14T20:19:04Z

packages/bns-codec/src/util.ts

+  a.reduce((acc: number, x: number, i: number) => (x === b[i] ? acc + 1 : acc), 0) === b.length;
+
+export const isHashIdentifier = (ident: Uint8Array): boolean =>
+  arraysEqual(HashId, ident.slice(0, HashId.length));


Is the slice helpful here? Seems counterproductive to the length check in arraysEqual.

ident will be longer, much longer. we just need to ensure the prefix matches.
i wanted to leave the other function as generic, as a compare should exist

will-iov · 2018-06-14T20:24:36Z

packages/bns-codec/tests/codec.spec.ts

+      await verify(swapClaimTxJson);
+      await verify(swapTimeoutTxJson);
+    } catch (err) {
+      expect(err).toBe(false);


Looks like you want .not.toThrow: https://jasmine.github.io/api/3.0/matchers.html#toThrow
(Although I'm not 100% how that works with await.)

will-iov · 2018-06-14T20:26:41Z

packages/bns-codec/tests/encode.spec.ts

+    // force result into Uint8Array for tests so it passes
+    // if buffer of correct type as well
+    expect(encoded.length).toEqual(pubBin.length);
+    expect(Uint8Array.from(encoded)).toEqual(pubBin);


This check makes the length check redundant.

webmaster128 · 2018-06-15T07:32:37Z

packages/bns-codec/tests/codec.spec.ts

+      await verify(swapClaimTxJson);
+      await verify(swapTimeoutTxJson);
+    } catch (err) {
+      expect(err).toBe(false);


What you want to say here is

} catch (err) { fail("Unexpected exception: " + err.message);

much better

webmaster128 · 2018-06-15T07:34:42Z

packages/bns-codec/tests/decode.spec.ts

+      expect(err.toString()).toContain("RangeError");
+      return;
+    }
+    expect(false).toBeTruthy();


DIdn't know this one....

webmaster128 · 2018-06-15T07:39:35Z

Let me sum up what is left from my point of view:

Rename export const HashId to export const hashId
arraysEqual implemented using every (see comment by Will)
Unittests for arraysEqual (copy/paste from above)
expect(err).toBe(false); -> fail()
expect(false).toBeTruthy(); -> fail() in multiple places
import Long from "long"; -> import * as Long from "long"; nowhere or everywhere. Not sure which one is the final answer

Now I just realized there are tests for arraysEqual, which looks a bit too complicated to me with all the hexing and slicing. Maybe merge our test sets.

ethanfrey · 2018-06-15T09:04:22Z

Okay, I fixed this up with all review comments.

The question about import Long from "long"; -> import * as Long from "long";
I tried the change, and the tsc fails.

The other change I made as tsc failed without the * as Long.
I think it has to do with the require('...').default or no default. I don't understand the deal, but until then I just give the compiler what it wants.
Maybe we can look into this later. (The other one I changed so I passed on karma tests)

ethanfrey changed the title ~~Add codec for BNS transactions~~ WIP: Add codec for BNS transactions Jun 12, 2018

ethanfrey requested review from webmaster128 and will-iov and removed request for webmaster128 June 12, 2018 19:32

webmaster128 reviewed Jun 13, 2018

View reviewed changes

ethanfrey force-pushed the bns-codec branch 2 times, most recently from 8440f36 to 685532b Compare June 13, 2018 15:42

ethanfrey force-pushed the bns-codec branch 3 times, most recently from c5e411c to ecc524a Compare June 13, 2018 18:52

webmaster128 suggested changes Jun 14, 2018

View reviewed changes

ethanfrey added 12 commits June 14, 2018 18:03

Set up bns-codec project

0994687

Generate codec from protobuf files

1151b76

Placeholder for final tx object

f524ee3

Start creating sendtx in protobuf

8a0ac4c

Got util functions compiling

3922baa

index builds sendtx

858b677

Create signbytes and txid as well as postbytes

e6018bc

Add constants for transaction types

d61f5af

add buildSetNameTx

7726454

Add swap offer/counter transactions

52aca74

Cover all tx types, return and release

54637ac

Fixed some imports to properly handle default

0612d8a

ethanfrey added 8 commits June 14, 2018 18:03

Cleanup util code

321fbcc

Removed obsolete control tests

a825d69

Split decode tests into own file

e496082

Improve BaseTx decoding

fa2cb1c

properly verify hash identifier on escrows

ee404ba

remove obsolete example code

0d9e2e2

New style: JSON -> Json

be6324b

Update interfaces for Signed/Unsigned Transaction update

2b883fd

ethanfrey force-pushed the bns-codec branch from 6211bde to 2b883fd Compare June 14, 2018 16:03

ethanfrey added 3 commits June 14, 2018 18:06

decodeAscii -> encodeAsAscii

4d28426

Add codec tests

4cf0de1

test serialization of all tx types, fix isHashIdentifier check

8feba49

ethanfrey changed the title ~~WIP: Add codec for BNS transactions~~ Add codec for BNS transactions Jun 14, 2018

will-iov reviewed Jun 14, 2018

View reviewed changes

webmaster128 reviewed Jun 15, 2018

View reviewed changes

ethanfrey added 3 commits June 15, 2018 10:45

HashId -> hashId

2caf237

Fix implementation and tests of arrayEquals

92e66a4

Cleanup test code

d24228f

ethanfrey force-pushed the bns-codec branch from 5c24969 to d24228f Compare June 15, 2018 09:01

webmaster128 approved these changes Jun 15, 2018

View reviewed changes

webmaster128 merged commit c74125b into master Jun 15, 2018

webmaster128 deleted the bns-codec branch June 15, 2018 09:11

This was referenced Jun 16, 2018

Modify SignBytes format iov-one/weave#71

Closed

Write TxCodec for bcp-demo #30

Closed

Web 4 Specification #2

Closed


		const { fromHex, toHex } = Encoding;

		describe("Control", () => {

Add codec for BNS transactions #56

Add codec for BNS transactions #56

Conversation

ethanfrey commented Jun 12, 2018 • edited Loading

ethanfrey commented Jun 12, 2018

ethanfrey commented Jun 12, 2018 • edited Loading

webmaster128 commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Jun 13, 2018

Isabello commented Jun 13, 2018

webmaster128 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

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

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Jun 14, 2018

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

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

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

webmaster128 Jun 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmaster128 commented Jun 15, 2018 • edited by ethanfrey Loading

ethanfrey commented Jun 15, 2018

ethanfrey commented Jun 12, 2018 •

edited

Loading

ethanfrey commented Jun 12, 2018 •

edited

Loading

webmaster128 Jun 15, 2018 •

edited

Loading

webmaster128 commented Jun 15, 2018 •

edited by ethanfrey

Loading