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

Wallet websocket covenant events #195

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

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Jun 11, 2019

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 event
  • namestate is the NameState json object
  • details is a transaction json with additional details such as the paths

Includes tests for events over websocket using bsock.

@@ -1303,6 +1303,21 @@ class HTTP extends Server {
this.to('w:*', event, wallet.id, json);
};

Copy link
Contributor Author

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

Copy link
Contributor Author

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-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

Merging #195 into master will decrease coverage by 0.1%.
The diff coverage is 76%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/wallet/txdb.js 77.97% <68.57%> (+0.09%) ⬆️
lib/wallet/http.js 65.49% <93.33%> (+0.45%) ⬆️
lib/primitives/coin.js 67.66% <0%> (-16.55%) ⬇️
lib/node/http.js 47.2% <0%> (-2.1%) ⬇️
lib/script/script.js 58% <0%> (-1.41%) ⬇️
lib/primitives/keyring.js 68.96% <0%> (-0.87%) ⬇️
lib/primitives/tx.js 72.96% <0%> (-0.44%) ⬇️
lib/hsd.js 97.26% <0%> (-0.21%) ⬇️
lib/mempool/mempool.js 42.68% <0%> (-0.15%) ⬇️
lib/net/framer.js 100% <0%> (ø) ⬆️
... and 7 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 0d5865f...b80bbc5. Read the comment docs.

lib/wallet/txdb.js Outdated Show resolved Hide resolved
this.emit(channel, {name, output, tx}, details);
break;
}
case types.REVEAL:
Copy link
Contributor Author

@tynes tynes Jun 11, 2019

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

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@boymanjor boymanjor left a 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.

test/wallet-http-test.js Show resolved Hide resolved
lib/wallet/txdb.js Outdated Show resolved Hide resolved
this.emit(channel, {name, output, tx}, details);
break;
}
case types.REVEAL:
Copy link
Contributor

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.

@tynes tynes added the wallet label Jun 21, 2019
@tynes tynes changed the title Wallet websocket covenant events WIP: Wallet websocket covenant events Jun 21, 2019
@tynes tynes force-pushed the wallet-covenant-events branch from f556748 to 9042ed9 Compare June 22, 2019 17:01
const nameHash = covenant.get(0);
const ns = await view.getNameState(this, nameHash);

if (EMPTY.equals(ns.name) || !ns.name) {
Copy link
Contributor Author

@tynes tynes Jun 22, 2019

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.

lib/wallet/txdb.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor Author

tynes commented Jun 22, 2019

As discussed out of band, I think we should emit events for tree updates as well.

PR for emitting tree updates #201, the node websocket topic "tree commit"

@tynes
Copy link
Contributor Author

tynes commented Jun 22, 2019

I don't think the additional i/o introduced by this PR is a problem because it first looks in the nameview that is populated in connectNames with a database lookup. It will only do another database lookup in the case that nameview does not already have the name. We could add a runtime flag to disable the event emitting codepath (including the additional i/o) if we wanted to, but I don't think its necessary.

@tynes tynes force-pushed the wallet-covenant-events branch from 9042ed9 to bf2842f Compare June 22, 2019 17:24
Mark Tyneway added 3 commits June 22, 2019 11:26
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.
@tynes tynes force-pushed the wallet-covenant-events branch from bf2842f to e007f14 Compare June 22, 2019 17:27
@tynes tynes changed the title WIP: Wallet websocket covenant events Wallet websocket covenant events Jun 22, 2019
lib/wallet/txdb.js Outdated Show resolved Hide resolved
@tynes tynes force-pushed the wallet-covenant-events branch from ae70b8d to b80bbc5 Compare August 13, 2019 08:59
@pinheadmz
Copy link
Member

concept ACK -- I'm gonna keep this on my to-review pile and maybe we can get it into v2.4.0

@nodech nodech added tests part of the codebase and removed api labels Nov 18, 2021
@nodech nodech added wallet part of the codebase wallet-api part of the codebase wallet-db part of the codebase ready-for-review modifier needs rebase modifier waiting for the author modifier and removed ready-for-review modifier labels Nov 18, 2021
@nodech nodech added breaking-minor Backwards compatible - Release version takeover Stale PRs. and removed waiting for the author modifier 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 needs rebase modifier takeover Stale PRs. tests part of the codebase wallet part of the codebase wallet-api part of the codebase wallet-db part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants