-
Notifications
You must be signed in to change notification settings - Fork 282
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
Wallet websocket covenant events #195
base: master
Are you sure you want to change the base?
Conversation
@@ -1303,6 +1303,21 @@ class HTTP extends Server { | |||
this.to('w:*', event, wallet.id, json); | |||
}; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably use handleTX
instead of creating a new function here, but the difference is in this.to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleTX
doesn't pass along enough arguments, we need to add another method or refactor handleTX
. The problem is that handleTX
does nothing with the tx
argument, and the covenant related events pass along the argument at that position. A new function called handleCovenant
was created.
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
==========================================
- Coverage 53.12% 53.02% -0.11%
==========================================
Files 129 129
Lines 35748 35795 +47
Branches 6022 6044 +22
==========================================
- Hits 18991 18980 -11
- Misses 16757 16815 +58
Continue to review full report at Codecov.
|
lib/wallet/txdb.js
Outdated
this.emit(channel, {name, output, tx}, details); | ||
break; | ||
} | ||
case types.REVEAL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these covenants, the name is not in the covenant data, it requires a db lookup to look up the name based on the hash. I don't think this will result in too much i/o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To alleviate the added i/o, we could refactor this.connectNames
. Currently, after processing incoming covenants, the method returns a boolean that indicates whether the name state was updated. We could instead have it return a summary object which accumulates all processed covenants.
We could then pass this summary object to this.emitCovenants
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #195 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed out of band, I think we should emit events for tree updates as well.
lib/wallet/txdb.js
Outdated
this.emit(channel, {name, output, tx}, details); | ||
break; | ||
} | ||
case types.REVEAL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To alleviate the added i/o, we could refactor this.connectNames
. Currently, after processing incoming covenants, the method returns a boolean that indicates whether the name state was updated. We could instead have it return a summary object which accumulates all processed covenants.
We could then pass this summary object to this.emitCovenants
.
f556748
to
9042ed9
Compare
const nameHash = covenant.get(0); | ||
const ns = await view.getNameState(this, nameHash); | ||
|
||
if (EMPTY.equals(ns.name) || !ns.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The view
accumulates state (the name
itself is what we want) during connectNames
. If the name
was added to the NameState
object, then skip the i/o involved with looking it up. Look it up in the covenant data if its present, otherwise query the wallet database for the name based on the nameHash
.
PR for emitting tree updates #201, the node websocket topic |
I don't think the additional i/o introduced by this PR is a problem because it first looks in the |
9042ed9
to
bf2842f
Compare
Add tests for socket events based on auction based transactions being indexed in the wallet txdb.
Add a new method `emitCovenants` that emits an event for each auction related output in a transaction that is indexed in the walletdb.
Emit events over websocket for auction related events that are emitted by the walletdb.
bf2842f
to
e007f14
Compare
ae70b8d
to
b80bbc5
Compare
concept ACK -- I'm gonna keep this on my to-review pile and maybe we can get it into v2.4.0 |
Emit websocket events for each type of covenant when the wallet indexes the transaction. This is implemented by iterating over each output in the transaction. The topic is:
${lowercase(type)} covenant
For example
bid covenant
.The payload is
(walletid, namestate, details)
walletid
is the id of the wallet that caused the eventnamestate
is theNameState
json objectdetails
is a transaction json with additional details such as the pathsIncludes tests for events over websocket using
bsock
.