Add improved local options with preflight #185

ggobugi27 · 2023-03-06T15:26:55Z

Reason:

kadena-io/chainweb-node#1585

Adds function to implement the new feature in chainweb, /local with preflight and signatureVerification

Changes made (preferably with images/screenshots):

chainweb-node-client

local defaults to preflight=true, signatureVerification=true
-local and send only accepts fully signed commands.
moved createSendRequest , createPollRequest , createListenRequest from kadena.js

kadena.js

prepareExecCommand , prepareContCommand, createCommand results changed from ICommand to IUnsignedCommand

types

removed {sig: undefined} option in ICommand
added IUnsignedCommand

pactjs

added isSignedCommand function which checks if signature is present
added ensureSignedCommand function which checks if signature is present and return the command in ICommand type, throws error if signature is not present.

Check off the following:

[V ] I have reviewed my changes and run the appropriate tests.
[ V] I have have run rush change to add the appropriate change logs.
[V ] I have added/edited docs.
I have added tutorials.
I have double checked and DEFINITELY added docs.

EnoF · 2023-03-08T12:48:12Z

packages/libs/chainweb-node-client/src/tests/send.test.ts

-import type { ICommand, SignatureWithHash } from '@kadena/types';
-
+import type { IUnsignedCommand, SignCommand } from '@kadena/types';
+import { createSendRequest } from 'kadena.js';


Should this import not be something along the lines of @kadena/client ?

@kadena/client 's send uses the send in chainweb-node-client, and has it's own send function from the command builder.

alber70g

Its a preliminary, but I wanted to get you started

packages/libs/chainweb-node-client/package.json

packages/libs/kadena.js/.eslintrc.js

packages/libs/chainweb-node-client/src/local.ts

packages/libs/chainweb-node-client/src/parseResponse.ts

packages/libs/chainweb-node-client/src/send.ts

packages/libs/chainweb-node-client/src/local.ts

packages/libs/client/package.json

packages/libs/kadena.js/src/api/createSendRequest.ts

alber70g · 2023-03-17T12:59:34Z

packages/libs/types/src/PactCommand.ts

+export interface ILocalCommandResultWithPreflight {
+  reqKey: IBase64Url;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  txId: number | null;
+  result: IPactResultSuccess | IPactResultError;
+  gas: number;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  logs: string | null;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  continuation: IPactExec | null;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  metaData: IChainwebResponseMetaData | null;
+  events?: Array<IPactEvent>;
+  preflightWarnings?: Array<string>;
+}


Would it be better to have a intersection type between ILocalCommandResult and the separate Preflight part, instead of duplicating across two interfaces?

per our discussion, because the preflight result only includes another field, preflightWarnings, this type will be used as the LocalResultWithPreflight , and LocalResultWithoutPreflight will be

Omit< LocalResultWithPreflight, 'preflightWarnings' >;```

alber70g

Some comments

…chainweb-node-client

Randynamic · 2023-04-05T13:20:44Z

@all-contributors please add @ggobugi27 for code

allcontributors · 2023-04-05T13:20:53Z

@Randynamic

I've put up a pull request to add @ggobugi27! 🎉

alber70g

Minor comments

alber70g · 2023-04-11T07:45:18Z

packages/libs/chainweb-node-client/src/interfaces/PactAPI.ts

+export type LocalResultWithoutPreflight = Omit<
+  LocalResultWithPreflight,
+  'preflightWarnings'
+>;


What is the default for a local call, with or without preflight? I'd rather have one that doesn't have a specifier on preflight. So LocalResult and either LocalPreflightResult or LocalResultWithoutPreflight.

The default will be with preflight. I'll change the naming.

alber70g · 2023-04-11T07:47:42Z

packages/libs/chainweb-node-client/src/interfaces/PactAPI.ts

+export interface ILocalCommandResultWithPreflight {
+  reqKey: IBase64Url;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  txId: number | null;
+  result: IPactResultSuccess | IPactResultError;
+  gas: number;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  logs: string | null;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  continuation: IPactExec | null;
+  /* eslint-disable-next-line @rushstack/no-new-null*/
+  metaData: IChainwebResponseMetaData | null;
+  events?: Array<IPactEvent>;
+  preflightWarnings?: Array<string>;
+}


I see on line 228 the following interface:

export interface IPreflightResult { preflightResult: ICommandResult; preflightWarnings: []; }

Should ILocalCommandResultWithPreflight also have preflightResult? Or is this what the API returns and not what is used in downstream code?

This one is interface for the parsed commandResult, where preflightResult is spread into the object.

packages/libs/chainweb-node-client/src/interfaces/PactAPI.ts

packages/libs/chainweb-node-client/src/parseResponse.ts

alber70g · 2023-04-11T08:35:23Z

packages/libs/chainweb-node-client/src/tests/spv.test.ts

 test('/spv returns SPV proof', async () => {
-  const actual: string | Response = await spv(testSPVRequest, '');
+  const actual: string | Response = await spv(testSPVRequest, testURL);
  const expected: SPVResponse = testSPVProof;


Could you make an issue to convert all tests to describe+it?

…rom chainwebnetworkid

ggobugi27 added 12 commits March 3, 2023 18:41

add preflight functions

0271c43

fix parsePreflight

e8294ca

debug unsignedCommand

d070676

separate ICommand and IUnsignedCommand

6c59916

add isSigned to crypto-utils and solve type error

a3f9a39

fix chainweb-node tests

499b8c6

fix local test

155178c

merge master

180cd0d

move back openapi

93d0dbe

delete comments

99ad9a7

order interfaces

d952fbe

remove duplicate interface

5e1272e

EnoF reviewed Mar 8, 2023

View reviewed changes

EnoF approved these changes Mar 8, 2023

View reviewed changes

ggobugi27 added 9 commits March 9, 2023 23:32

resolve type errors

fbb85f0

resolve errors, put back ISendResponse in chainweb-client-node

a4a50e4

add localWithoutSignatureVerification in client

538e17c

throw error when unsigned command is passed to

54cc773

run rush change

b318f84

fix chainweb-node-client test

e8f6ef7

fix client tets

ab9f2ec

add test coverage in client, crypto-utils

2c8bd30

add @Alpha tag

30136d6

ggobugi27 requested a review from alber70g March 10, 2023 11:54

ggobugi27 changed the title ~~[WIP] Add improved local options with preflight~~ Add improved local options with preflight Mar 10, 2023

ggobugi27 requested review from Randynamic, jermaine150 and nillo March 10, 2023 11:58

alber70g approved these changes Mar 16, 2023

View reviewed changes

alber70g approved these changes Mar 17, 2023

View reviewed changes

alber70g self-requested a review March 17, 2023 12:59

alber70g requested changes Mar 17, 2023

View reviewed changes

ggobugi27 added 10 commits March 23, 2023 17:21

remove localWithoutPreflight

0721970

remove localWithoutPreflight, move API types and request-creators to …

fab38c1

…chainweb-node-client

debug options

c618904

debugging state that passes test with preflight:true

da9ec39

fix overload matching error

b7cf360

fix tests

2b709ad

run rush change

a2ead43

merge master

9108b4a

change ISendResponse to SendResponse

08ab890

add tests for pactjs

7027dfb

allcontributors bot mentioned this pull request Apr 5, 2023

docs: add ggobugi27 as a contributor for code #216

Merged

alber70g approved these changes Apr 11, 2023

View reviewed changes

ggobugi27 and others added 4 commits April 11, 2023 18:47

rename localResultWithPreflight to localResult and remove undefined f…

7c0d829

…rom chainwebnetworkid

chore(client): refactored LocalResponse is to be ILocalCommandResult

4e8269f

rush update

28d70c9

rename LocalResponse to ILocalCommandResult

6b72d34

ggobugi27 merged commit 46a356e into master Apr 11, 2023

ggobugi27 deleted the feat/local branch April 11, 2023 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add improved local options with preflight #185

Add improved local options with preflight #185

ggobugi27 commented Mar 6, 2023 •

edited

Loading

EnoF Mar 8, 2023

ggobugi27 Mar 10, 2023

alber70g left a comment

alber70g Mar 17, 2023

ggobugi27 Apr 4, 2023

alber70g left a comment

Randynamic commented Apr 5, 2023

allcontributors bot commented Apr 5, 2023

alber70g left a comment

alber70g Apr 11, 2023

ggobugi27 Apr 11, 2023

alber70g Apr 11, 2023

ggobugi27 Apr 11, 2023

alber70g Apr 11, 2023

Add improved local options with preflight #185

Add improved local options with preflight #185

Conversation

ggobugi27 commented Mar 6, 2023 • edited Loading

Reason:

Changes made (preferably with images/screenshots):

Check off the following:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alber70g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alber70g left a comment

Choose a reason for hiding this comment

Randynamic commented Apr 5, 2023

allcontributors bot commented Apr 5, 2023

alber70g 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

ggobugi27 commented Mar 6, 2023 •

edited

Loading