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

Node http endpoints and handlers #168

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

wi-ski
Copy link
Contributor

@wi-ski wi-ski commented May 5, 2019

Change Summary & Context

The following changes implement the routes outlined in the specification here: #142 (comment)

This PR is intended to be tested using this branch of hs-client:

They are covered by tests here: https://github.com/handshake-org/hsd/pull/168/files#diff-bbe506ab5582252a30d6872d35fd8ce6L21

package.json Outdated Show resolved Hide resolved
@wi-ski
Copy link
Contributor Author

wi-ski commented May 5, 2019

Hey @tynes we spoke about landing these changes are separate PRs. But there were some consistency/non-determinism issues I thought we should chat about first!

test/node-http-test.js Outdated Show resolved Hide resolved
@chjj
Copy link
Contributor

chjj commented May 13, 2019

Good work. This is probably a good start to adding names to the rest API. Will investigate the other race conditions tomorrow.

@tynes
Copy link
Contributor

tynes commented May 14, 2019

@wi-ski - I have an open PR here for GET /info/name/:name #162

I'd happily close it if you got this PR cleaned up

@wi-ski wi-ski changed the title Node http nameinfo Node http endpoints and handlers May 14, 2019
@wi-ski
Copy link
Contributor Author

wi-ski commented May 14, 2019

@tynes - Sitting down to take a pass at this now. Quick question: You ok with these changes landing in a single PR? 👼

@wi-ski
Copy link
Contributor Author

wi-ski commented May 14, 2019

@tynes @chjj - Hey dudes. I took a stab at getting an event-listening setup working but encountered some errors using the .on(...) method of the FullNode. I'm at a loss.

Additionally, I tried to implement various forms of a sleep-esque test util and found that it actually made the test pass less consistently. This has been a battle.

I can set aside some to see if there's a better way to listen for events. @tynes - If you feel like any of this usable, please have at it.

🤔 I really was stumped, listening for events would just mysteriously error most of the time. Super frustrating \o.o/.

test/node-http-test.js Outdated Show resolved Hide resolved
test/node-http-test.js Outdated Show resolved Hide resolved
@wi-ski
Copy link
Contributor Author

wi-ski commented May 14, 2019

Oh oh, I didn't even mention - I cleaned up the PR in that I:

  1. Swapped out the "named" methods that were being used on the updated hs-client (using .get(...) now)
  2. Restored package.json

Is there a recommended way of "sleeping" that either of you have had success with btw?

test/node-http-test.js Outdated Show resolved Hide resolved
test/node-http-test.js Outdated Show resolved Hide resolved
test/node-http-test.js Outdated Show resolved Hide resolved
test/node-http-test.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented May 14, 2019

A simple sleep function:

async function sleep(time) {
  return new Promise(resolve => setTimeout(resolve, time));
}

@tynes
Copy link
Contributor

tynes commented May 14, 2019

More useful helper functions by @braydonf

https://github.com/bcoin-org/bcoin/pull/748/files#diff-e169f8c8ca803ee76312e094ba6f17cbR105

common.event = async function event(obj, name) {
  return new Promise((resolve) => {
    obj.once(name, resolve);
  });
};

https://github.com/bcoin-org/bcoin/pull/758/files#diff-e169f8c8ca803ee76312e094ba6f17cbR105

common.forValue = async function(obj, key, val, timeout = 30000) {
  assert(typeof obj === 'object');
  assert(typeof key === 'string');

  const ms = 10;
  let interval = null;
  let count = 0;
  return new Promise((resolve, reject) => {
    interval = setInterval(() => {
      if (obj[key] === val) {
        clearInterval(interval);
        resolve();
      } else if (count * ms >= timeout) {
        clearInterval(interval);
        reject(new Error('Timeout waiting for value.'));
      }
      count += 1;
    }, ms);
  });
};

@boymanjor
Copy link
Contributor

@tynes - Sitting down to take a pass at this now. Quick question: You ok with these changes landing in a single PR? 👼

@tynes @wi-ski I think this is fine as one, name related PR.

@codecov-io
Copy link

codecov-io commented May 15, 2019

Codecov Report

Merging #168 into master will increase coverage by 0.65%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   52.99%   53.64%   +0.65%     
==========================================
  Files         129      129              
  Lines       35773    35845      +72     
  Branches     6032     6043      +11     
==========================================
+ Hits        18957    19230     +273     
+ Misses      16816    16615     -201
Impacted Files Coverage Δ
lib/node/http.js 55.8% <91.66%> (+6.5%) ⬆️
lib/covenants/rules.js 73.04% <0%> (-0.15%) ⬇️
lib/net/pool.js 24.22% <0%> (+0.04%) ⬆️
lib/script/script.js 58% <0%> (+0.07%) ⬆️
lib/net/peer.js 20.06% <0%> (+0.3%) ⬆️
lib/blockchain/chaindb.js 64.1% <0%> (+0.37%) ⬆️
lib/wallet/wallet.js 62.4% <0%> (+0.99%) ⬆️
lib/wallet/nodeclient.js 80% <0%> (+1.42%) ⬆️
lib/covenants/namestate.js 88.57% <0%> (+1.6%) ⬆️
... and 4 more

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 cc1ef7a...231e30e. Read the comment docs.

@wi-ski
Copy link
Contributor Author

wi-ski commented May 15, 2019

Ok - extending the timeout for the test suite allowed for these to pass. Not ideal.

@tynes
Copy link
Contributor

tynes commented May 15, 2019

Can you mine less blocks? We don't want to dramatically extend the length of the test suite with just one set of tests

@wi-ski
Copy link
Contributor Author

wi-ski commented May 15, 2019

@tynes - Updates:

  1. Using the mentioned event listening util.
    • Test pass seemingly consistently.
  2. Mining fewer blocks.

Recent CI passed with longer test times requiring a little under 10 seconds to complete.

Hit me with your feedback when you can.

@wi-ski
Copy link
Contributor Author

wi-ski commented May 23, 2019

#bump. @tynes, anything I can do for this PR to help get it over the line?

lib/node/http.js Outdated
if (!ns)
return res.json(404);

return res.json(200, { name: ns.name.toString('binary') });
Copy link
Contributor

@tynes tynes May 23, 2019

Choose a reason for hiding this comment

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

ns.getJSON should be the 2nd argument to res.json like this:

return ns.getJSON(height, network);

See implementation here:

https://github.com/handshake-org/hsd/blob/master/lib/covenants/namestate.js#L651

lib/node/http.js Outdated

const resource = Resource.decode(ns.data);

return res.json(200, resource.toJSON());
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass in the name to getJSON, see here:

getJSON(name) {

lib/node/http.js Outdated
const proof = await this.chain.db.prove(root, key);

return res.json(200, {
hash: hash.toString('hex'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Its not particularly clear what hash is in this context, what about tip? It is used this way for node GET /

tip: this.chain.tip.hash.toString('hex'),

I think key should be renamed to nameHash

@tynes
Copy link
Contributor

tynes commented May 23, 2019

@wi-ski thanks for the bump. Just left a few comments for you

@wi-ski wi-ski force-pushed the node-http-nameinfo branch from a349d2d to 9050228 Compare June 27, 2019 06:40
@wi-ski
Copy link
Contributor Author

wi-ski commented Jun 29, 2019

@tynes #bump

@@ -113,6 +117,20 @@ common.forValue = async function(obj, key, val, timeout = 30000) {
});
};

common.constructBlockMiner = function (node, nclient) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can feel others thinking this is less than optimal - the whole "pass me a full node and api client" signature is wonky 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a better way besides using a closure like this, can you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope!

@@ -79,6 +79,7 @@ describe('Wallet HTTP', function() {
});

beforeEach(async () => {
mineBlocks = common.constructBlockMiner(node, nclient);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to do this before every it block do we? Its always the same node and the same client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope!

@@ -91,6 +91,10 @@ common.event = async function event(obj, name) {
});
};

common.sleep = function sleep(time) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call on moving sleep here

return res.json(200, resource.getJSON(name));
});

this.get('/proof/name/:name', async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, there are different kinds of proofs. There is an Urkle Tree Proof, which authenticates the namestate and then there is a Merkle Proof for transactions in a block. Ideally we want to be able to support both of them and if we use /proof/name/:name then we can use /proof/tx/:txid, but I think that would only work if transaction indexing is turned on. So using /proof/name/:name works.

lib/node/http.js Outdated
root: root.toString('hex'),
name: name,
key: nameHash.toString('hex'),
proof: proof.toJSON()
Copy link
Contributor

Choose a reason for hiding this comment

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

The shape of proof.toJSON looks like:

  toJSON() {
    return {
      type: typesByVal[this.type],
      depth: this.depth,
      nodes: this.nodes.map(node => node.toJSON()),
      prefix: this.prefix ? this.prefix.toString() : undefined,
      left: this.left ? this.left.toString('hex') : undefined,
      right: this.right ? this.right.toString('hex') : undefined,
      key: this.key ? this.key.toString('hex') : undefined,
      hash: this.hash ? this.hash.toString('hex') : undefined,
      value: this.value ? this.value.toString('hex') : undefined
    };
  }

We should try to deduplicate any fields that correspond between the two. It feels a little bit cleaner to me to follow the style in other endpoints where the only object returned is a primitive.toJSON.

We want to be 100% sure that we can easily verify the proof client side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 I dont entirely follow.

This shape mirrors the response of the RPC method:
https://github.com/handshake-org/hsd/blob/master/lib/node/rpc.js#L2375

Would you prefer something like:

    return {
      hash: hash.toString('hex'),
      height: height,
      root: root.toString('hex'),
      name: name,
      key: key.toString('hex'),
      ...proof.toJSON() // <- This would achieve your de-duping.
    };

It is definitely cleaner to return something.toJSON() - but to confirm, we have other handlers in this file that construct their response "in the handler" like: https://github.com/handshake-org/hsd/blob/master/lib/node/http.js#L354

Whichever we decide - Illl do the same for: https://github.com/handshake-org/hsd/pull/168/files/231e30eef2c442d4914c57cddd840a03624abac9#diff-bbe506ab5582252a30d6872d35fd8ce6R540

Copy link
Contributor Author

@wi-ski wi-ski Jul 3, 2019

Choose a reason for hiding this comment

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

Current question(s) I believe Im trying to answer:
a. Do we want to return the shape of proof.toJSON() with height & root & name included?
or
b. Something else that results in something akin to:

      return res.json(200, primitive.toJSON()); // Like proof.toJSON?

Copy link
Contributor

Choose a reason for hiding this comment

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

but to confirm, we have other handlers in this file that construct their response "in the handler"

This one was done that way because there was no filter.JSON and the filter object lives in a different repo and its hard to coordinate PRs in multiple repos

Copy link
Contributor

Choose a reason for hiding this comment

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

Between a and b, I think something in between is good, it would be nice to have the height and the root along with the rest of proof.toJSON. What we really need to be 100% sure of is that we have everything that we need client side to verify the proof, which I believe proof.toJSON gives.

Copy link
Contributor Author

@wi-ski wi-ski Jul 24, 2019

Choose a reason for hiding this comment

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

I added the:

    return {
      hash: hash.toString('hex'),
      height: height,
      root: root.toString('hex'),
      name: name,
      key: key.toString('hex'),
      ...proof.toJSON()
    };

☝️ This makes sense to me? Let me know if you feel otherwise!


await mineBlocks(blocksUntilClose, cbAddress);

await wclient.execute('sendupdate', [NAME0, { compat: false, version: 0, ttl: 172800, ns: ['ns1.example.com.@1.2.3.4'] }]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is a little too long, can you turn the record into its own object? It would also be nice to assert that getting the namestate reflects the record.

});
});

describe('getNameProof', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could dedupe getnameproof and getnameproofbyhash with a loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Yess - we could. I don't think it would improve anything however. Do you feel strongly about this?

Copy link
Contributor Author

@wi-ski wi-ski Jul 24, 2019

Choose a reason for hiding this comment

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

There is solid overlap - but there's only "2 things" to abstract, this feels like a situation I'd want to hold off on abstracting into a loop. Your call. But I wanted to voice the thought 👍

@wi-ski
Copy link
Contributor Author

wi-ski commented Jul 24, 2019

@tynes:
a. Addressed line length issues.
b. The shape of the "proof" routes' response - I went with the:

      return res.json(200, {
        hash: tip.toString('hex'),
        height: height,
        root: root.toString('hex'),
        key: nameHash.toString('hex'),
        name: ns ? ns.name.toString() : null,
        ...proof.toJSON()
      });

approach. I think this achieves what I feel like you described as acceptable. It would be awesome to chat about that in more detail if you have any issues with it.
c. Loops in the test - I left a note about wanting to avoid this, it feels premature. If you feel otherwise after me bringing that up, let me know!

@wi-ski
Copy link
Contributor Author

wi-ski commented Jul 31, 2019

@tynes bump <3

@wi-ski
Copy link
Contributor Author

wi-ski commented Aug 9, 2019

@tynes - Touching base on this. How are we looking?

Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Left out a few duplicate comments but overall its good. Would be ideal to squash the commits, sign them and follow the commit message schema for the project

const name = valid.str('name');

if (!name || !rules.verifyName(name))
throw new Error('Invalid parameter.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: 'Invalid name.'


const nameHash = rules.hashName(name);
const ns = await this.chain.db.getNameState(nameHash);

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to return null here, we want to return a res.json(404)

const height = this.chain.tip.height;
const network = this.network;

if (!ns)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to check if ns.data.length === 0

root: root.toString('hex'),
name: name,
key: nameHash.toString('hex'),
...proof.toJSON()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this solution, I think it would be more ideal to pass some data into proof.toJSON but then that would couple the proof to a particular blockchain implementation when it could be agnostic. The proof is defined in the urkel library, not hsd.

Maybe something like:

{
  proof: proof.toJSON(),
  chain: {
    height: height
    root: root.toString('hex')
    name: name
  }

We want to be able to support passing in an arbitrary height (create proofs at a certain height) eventually, that feels like it could be its own endpoint GET /proof/name/:name/height/:height or by adding a query param GET /proof/name/:name?height=height

Pinging @boymanjor @pinheadmz for opinions

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to opinionate too much since I'm new to this thread but... To compare, rpc gettxoutproof just returns a raw hex string, and has a matching verifier rpc verifytxoutproof. I think getting a proof is useless (?) without a matching endpoint to verify it, and that should perhaps dictate the output format of the get proof endpoint?

Briefly checking this:

https://github.com/handshake-org/urkel/blob/c29466104b25f85ec2ef7a5025af625a6df21eda/lib/radix/proof.js#L256

verify(root, key, hash, bits) {...

... I would expect a proof-getter to return the proof (either JSON or serialized hex) along with the current (by default) tree root and perhaps the key, which is just the name hash, but the user might not have the hash if they are using this endpoint.

});
});

this.get('/proof/hash/:nameHash', async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: nameHash -> hash

await mineBlocks(1, cbAddress);

const resource = await nclient.get(`/resource/name/${NAME0}`);
assert.deepEqual(resource, Object.assign(records, { name: NAME0, version: 0 }));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for not doing a deepEqual on the result of an Object.assign and instead just comparing the object properties one by one. Then this test will not break if additional information is added.

describe('getNameProof', () => {
it('It should return a proof type of TYPE_DEADEND when an auction has not been initiated', async () => {
const proof = await nclient.get(`/proof/name/${NAME0}`);
assert.equal(proof.type, 'TYPE_DEADEND');
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.equal(proof.name, NAME0);
});

describe('When an auction has been initiated', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to nest this describe?

assert.equal(name, null);
});

describe('When an auction has been initiated', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to nest describe blocks?

});
});

describe('getNameProofByHash', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Get Name Proof By Hash

@tynes tynes mentioned this pull request Nov 1, 2019
25 tasks
@nodech nodech added breaking-minor Backwards compatible - Release version node-http part of the codebase tests part of the codebase labels Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-minor Backwards compatible - Release version node-http part of the codebase tests part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants