Node http endpoints and handlers #168
Conversation
|
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! |
|
Good work. This is probably a good start to adding names to the rest API. Will investigate the other race conditions tomorrow. |
|
@tynes - Sitting down to take a pass at this now. Quick question: You ok with these changes landing in a single PR? 👼 |
|
@tynes @chjj - Hey dudes. I took a stab at getting an event-listening setup working but encountered some errors using the 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/. |
|
Oh oh, I didn't even mention - I cleaned up the PR in that I:
Is there a recommended way of "sleeping" that either of you have had success with btw? |
|
A simple sleep function: async function sleep(time) {
return new Promise(resolve => setTimeout(resolve, time));
} |
|
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);
});
}; |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
Ok - extending the timeout for the test suite allowed for these to pass. Not ideal. |
|
Can you mine less blocks? We don't want to dramatically extend the length of the test suite with just one set of tests |
|
@tynes - Updates:
Recent CI passed with longer test times requiring a little under 10 seconds to complete. Hit me with your feedback when you can. |
|
#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') }); |
There was a problem hiding this comment.
ns.getJSON should be the 2nd argument to res.json like this:
Line 1774 in 696dfc4
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()); |
There was a problem hiding this comment.
Pass in the name to getJSON, see here:
Line 1309 in 696dfc4
lib/node/http.js
Outdated
| const proof = await this.chain.db.prove(root, key); | ||
|
|
||
| return res.json(200, { | ||
| hash: hash.toString('hex'), |
There was a problem hiding this comment.
Its not particularly clear what hash is in this context, what about tip? It is used this way for node GET /
Line 126 in 41ff917
I think key should be renamed to nameHash
|
@wi-ski thanks for the bump. Just left a few comments for you |
wi-ski
force-pushed
the
node-http-nameinfo
branch
from
a349d2d
to
9050228
Compare
|
@tynes #bump |
| @@ -113,6 +117,20 @@ common.forValue = async function(obj, key, val, timeout = 30000) { | |||
| }); | |||
| }; | |||
|
|
|||
| common.constructBlockMiner = function (node, nclient) { | |||
There was a problem hiding this comment.
I can feel others thinking this is less than optimal - the whole "pass me a full node and api client" signature is wonky 🤔
There was a problem hiding this comment.
I can't think of a better way besides using a closure like this, can you?
| @@ -79,6 +79,7 @@ describe('Wallet HTTP', function() { | |||
| }); | |||
|
|
|||
| beforeEach(async () => { | |||
| mineBlocks = common.constructBlockMiner(node, nclient); | |||
There was a problem hiding this comment.
We don't need to do this before every it block do we? Its always the same node and the same client
| @@ -91,6 +91,10 @@ common.event = async function event(obj, name) { | |||
| }); | |||
| }; | |||
|
|
|||
| common.sleep = function sleep(time) { | |||
There was a problem hiding this comment.
Good call on moving sleep here
| return res.json(200, resource.getJSON(name)); | ||
| }); | ||
|
|
||
| this.get('/proof/name/:name', async (req, res) => { |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🤔 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
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
test/node-http-test.js
Outdated
|
|
||
| await mineBlocks(blocksUntilClose, cbAddress); | ||
|
|
||
| await wclient.execute('sendupdate', [NAME0, { compat: false, version: 0, ttl: 172800, ns: ['ns1.example.com.@1.2.3.4'] }]); |
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
Do you think we could dedupe getnameproof and getnameproofbyhash with a loop?
There was a problem hiding this comment.
🤔 Yess - we could. I don't think it would improve anything however. Do you feel strongly about this?
There was a problem hiding this comment.
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 👍
|
@tynes: 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. |
|
@tynes bump <3 |
|
@tynes - Touching base on this. How are we looking? |
| const name = valid.str('name'); | ||
|
|
||
| if (!name || !rules.verifyName(name)) | ||
| throw new Error('Invalid parameter.'); |
|
|
||
| const nameHash = rules.hashName(name); | ||
| const ns = await this.chain.db.getNameState(nameHash); | ||
|
|
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Also need to check if ns.data.length === 0
| root: root.toString('hex'), | ||
| name: name, | ||
| key: nameHash.toString('hex'), | ||
| ...proof.toJSON() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
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) => { |
| await mineBlocks(1, cbAddress); | ||
|
|
||
| const resource = await nclient.get(`/resource/name/${NAME0}`); | ||
| assert.deepEqual(resource, Object.assign(records, { name: NAME0, version: 0 })); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
Nit: assertion against the enums defined here https://github.com/handshake-org/urkel/blob/fbc93f6fbfeabaf15c1bb36b3f5f7d64ee4c240e/lib/radix/proof.js#L664
| assert.equal(proof.name, NAME0); | ||
| }); | ||
|
|
||
| describe('When an auction has been initiated', () => { |
There was a problem hiding this comment.
Any reason to nest this describe?
| assert.equal(name, null); | ||
| }); | ||
|
|
||
| describe('When an auction has been initiated', () => { |
There was a problem hiding this comment.
Any reason to nest describe blocks?
| }); | ||
| }); | ||
|
|
||
| describe('getNameProofByHash', () => { |
nodech
added
breaking-minor
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