batch open transaction support on full node #34

rozaydin · 2020-11-25T08:10:24Z

Signed-off-by: rozaydin ridvan@namebase.io

Signed-off-by: rozaydin <ridvan@namebase.io>

coveralls · 2020-11-25T10:27:56Z

Pull Request Test Coverage Report for Build 384911669

77 of 91 (84.62%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 59.431%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/primitives/airdropkey.js	0	1	0.0%
lib/wallet/txdb.js	11	13	84.62%
lib/wallet/http.js	18	21	85.71%
lib/wallet/wallet.js	47	55	85.45%

Totals
Change from base Build 305332224:	0.07%
Covered Lines:	19500
Relevant Lines:	30560

💛 - Coveralls

Signed-off-by: rozaydin <ridvan@namebase.io>

rozaydin · 2020-11-25T11:29:18Z

i did struggle a little bit to understand the github flow and dependency management. but here is the main change i have for the batchOpen. I do have postman scripts and completed manual testing.

turbomaze

Good job with this PR, you got to touch a bunch of different parts of the Handshake wallet. Some overall comments:

Your .eslinting seems to be different than what this repo wants. Check out the .eslintrc files here. Can ask me if you need help on this, I stopped commenting on deviations after a certain point
Can you make this PR against production-namebase instead of master? Here's our branching strategy:

namebase   | handshake-org
-----------+--------------
master     | master
randomx    | randomx
production | v2.2.0 // production points to a specific commit of hsd
production-namebase // has no analog in hsd; it has our custom changes

So anything custom we do is on production-namebase, which is branched off of production. You will need to rebase this branch off of production-namebase which might lead to some rebase conflicts

turbomaze · 2020-11-25T23:12:37Z

lib/wallet/http.js

+
+      const options = TransactionOptions.fromValidator(valid);
+      const {mtx, errors, isAllError} = await req.wallet
+      .createBatchOpen(names, force, options);


This indentation does not seem correct, what eslint rules are you using?

turbomaze · 2020-11-25T23:13:49Z

lib/dns/resource.js

@@ -880,7 +880,7 @@ function typeToClass(type) {
 function stringToClass(type) {
  assert(typeof type === 'string');

-  if (!hsTypes.hasOwnProperty(type))
+  if (!Object.prototype.hasOwnProperty.call(hsTypes, type))


I think we want to avoid directly accessing prototypes, is your eslint config using some global rules defined elsewhere?

FWIW to avoid doing x.hasOwnProperty(y), you can do: Object.keys(x).includes(y) for better safety

turbomaze · 2020-11-25T23:13:55Z

lib/primitives/airdropkey.js

@@ -363,7 +363,7 @@ class AirdropKey extends bio.Struct {
  fromJSON(json) {
    assert(json && typeof json === 'object');
    assert(typeof json.type === 'string');
-    assert(keyTypes.hasOwnProperty(json.type));
+    assert(Object.prototype.hasOwnProperty.call(keyTypes, json.type));


Same comment

turbomaze · 2020-11-25T23:20:58Z

lib/wallet/http.js

+      const {mtx, errors, isAllError} = await req.wallet
+      .createBatchOpen(names, force, options);
+
+      if (isAllError) { // no valid output in mtx


Personally I don't like how hsd eslint rules want:

if (isAllError) return res.json(500, { errors: errors });

I think this --^ is very prone to bugs and don't like it, I prefer what you wrote, but, it's the convention in this repo, so could you change this if statement to that format? This should happen automatically with the rest of the eslint fixes in this PR

turbomaze · 2020-11-25T23:37:46Z

lib/wallet/txdb.js

+   * @returns {Boolean}
+   */
+
+  async isCovenantDoubleOpen(covenant) {


Few comments:

It seems like this is copied from isDoubleOpen(tx), correct? Could you rewrite isDoubleOpen(tx) so that it uses this isCovenantDoubleOpen(covenant) function?

Reading this code actually makes me think that there is a bug with the original isDoubleOpen that you copied it from! In the original, we return false from inside the for loop if the duplicate open is from an old auction. But that doesn't mean the whole transaction doesn't have a double open, just that covenant isn't! It should continue and not return false.

We can rewrite isDoubleOpen as something like:

async isDoubleOpen(tx) { const isDoubleOpenList = await Promise.all( tx.outputs.map(({ covenant }) => isCovenantDoubleOpen(covenant)) ); // any covenant being a double open means the whole transaction includes a double open return isDoubleOpenList.some(Boolean); }

Which fixes the continue bug.

Reviewing this code also caused me to review removeDoubleOpen(tx). Handshake used to have a bug that prevented valid double opens (when auction expires). isDoubleOpen fixed that bug, but removeDoubleOpen never did! It seems you're not using this removeDoubleOpen function anyway so no change necessary in this PR, just pointing it out.

removeDoubleOpen will keep it in mind, i am refactoring as you have indicated. I didnt realize the "bug" you mentioned above tough, addressing that one as well.

turbomaze · 2020-11-25T23:41:30Z

lib/wallet/wallet.js

+   * @returns {MTX}
+   */
+
+  async makeBatchOpen(names, force, acct) {


Can you rewrite all the other makeOpen/createOpen etc calls so that they are a special case of xxxBatchOpen? I want to avoid having bug fixes in one not translate to bug fixes in the other. Perhaps something like:

async makeOpen(name, force, acct) { const { mtx, errors, isAllError } = await makeBatchOpen([name], force, acct); if (errors.length > 0) { throw new Error(errors[0].error); } return mtx; }

turbomaze · 2020-11-25T23:42:25Z

lib/wallet/wallet.js

@@ -1559,6 +1559,87 @@ class Wallet extends EventEmitter {
    }
  }

+  /**
+   * Make a batch open MTX.
+   * @param {Array} names


Can you replace with string[] or Array<string> instead

turbomaze · 2020-11-25T23:42:49Z

lib/wallet/wallet.js

+
+    for (const name of names) {
+      if (!rules.verifyName(name)) {
+        errors.push({name: name, error: 'Invalid name.'});


The key error makes this sound like a typed object, could you call it message or errorMessage?

turbomaze · 2020-11-25T23:43:55Z

test/wallet-http-test.js

+  it('should create a batch open transaction (multiple outputs) for valid names', async () => {
+    // add funds to account, these are necessary to run test individually
+    // const height = 2;
+    // await mineBlocks(height, cbAddress);


Can we remove these comments?

turbomaze · 2020-11-25T23:44:36Z

test/wallet-http-test.js

+    assert.ok(mempool.length === 0);
+  });
+
+  it('should reject a batch open transaction (multiple outputs) for invalid names', async () => {


Great test coverage!

Signed-off-by: rozaydin <ridvan@namebase.io>

batch open transaction support on full node

9286cad

Signed-off-by: rozaydin <ridvan@namebase.io>

rozaydin requested review from RevCBH and turbomaze November 25, 2020 08:11

rozaydin assigned turbomaze Nov 25, 2020

rozaydin removed request for RevCBH and turbomaze November 25, 2020 08:13

rozaydin unassigned turbomaze Nov 25, 2020

rozaydin added 3 commits November 25, 2020 12:39

eslint changes for the pipeline

7bafbe2

Signed-off-by: rozaydin <ridvan@namebase.io>

changed github dependency url in package.json

d9f56be

Signed-off-by: rozaydin <ridvan@namebase.io>

updated github url in dependencies again

05c5ed3

Signed-off-by: rozaydin <ridvan@namebase.io>

rozaydin added 3 commits November 25, 2020 13:48

reset the wallet.js

be99b0a

Signed-off-by: rozaydin <ridvan@namebase.io>

reverted prettier changes that created so many changes in the file

28106fb

Signed-off-by: rozaydin <ridvan@namebase.io>

added missing methods after reset

44a4b79

Signed-off-by: rozaydin <ridvan@namebase.io>

rozaydin requested review from RevCBH and turbomaze November 25, 2020 11:27

rozaydin assigned turbomaze Nov 25, 2020

turbomaze suggested changes Nov 25, 2020

View reviewed changes

removed comments from the test cases

b43bb89

Signed-off-by: rozaydin <ridvan@namebase.io>

turbomaze closed this Dec 10, 2020

turbomaze deleted the ridvan-batch-open-transaction-support branch December 10, 2020 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batch open transaction support on full node #34

batch open transaction support on full node #34

rozaydin commented Nov 25, 2020

coveralls commented Nov 25, 2020 •

edited

Loading

rozaydin commented Nov 25, 2020

turbomaze left a comment

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

rozaydin Nov 26, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

turbomaze Nov 25, 2020

batch open transaction support on full node #34

batch open transaction support on full node #34

Conversation

rozaydin commented Nov 25, 2020

coveralls commented Nov 25, 2020 • edited Loading

Pull Request Test Coverage Report for Build 384911669

💛 - Coveralls

rozaydin commented Nov 25, 2020

turbomaze 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

coveralls commented Nov 25, 2020 •

edited

Loading