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

add "GetMempoolEntriesByAddresses" to kaspad RPC. #2022

Merged
merged 9 commits into from
Apr 26, 2022

Conversation

D-Stacks
Copy link
Contributor

@D-Stacks D-Stacks commented Apr 15, 2022

implements #1703
needed to fix #1984

Also hoping it will lead to other transaction and wallet improvements.

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #2022 (ab012e6) into dev (beb038c) will increase coverage by 0.06%.
The diff coverage is 74.56%.

@@            Coverage Diff             @@
##              dev    #2022      +/-   ##
==========================================
+ Coverage   59.39%   59.46%   +0.06%     
==========================================
  Files         671      675       +4     
  Lines       32028    32201     +173     
==========================================
+ Hits        19023    19147     +124     
- Misses      10276    10306      +30     
- Partials     2729     2748      +19     
Impacted Files Coverage Δ
app/appmessage/message.go 71.42% <ø> (ø)
app/rpc/rpc.go 63.88% <ø> (ø)
cmd/kaspactl/commands.go 0.00% <ø> (ø)
...rk/rpcclient/rpc_get_mempool_entries_by_address.go 45.45% <45.45%> (ø)
...ork/netadapter/server/grpcserver/protowire/wire.go 42.80% <66.66%> (+0.36%) ⬆️
.../protowire/rpc_get_mempool_entries_by_addresses.go 69.04% <69.04%> (ø)
...pc/rpchandlers/get_mempool_entries_by_addresses.go 85.71% <85.71%> (ø)
...appmessage/rpc_get_mempool_entries_by_addresses.go 100.00% <100.00%> (ø)
app/protocol/flows/v5/blockrelay/block_locator.go 52.94% <0.00%> (-11.77%) ⬇️
...ain/consensus/database/binaryserialization/hash.go 69.56% <0.00%> (-8.70%) ⬇️
... and 2 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 beb038c...ab012e6. Read the comment docs.

@D-Stacks D-Stacks changed the title add "GetMempoolEntriesByAddress" to kaspad RPC. add "GetMempoolEntriesByAddresses" to kaspad RPC. Apr 15, 2022
}
}

if len(sending) > 0 || len(receiving) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes more sense to drop this if. This way the index for each address entries will be predictable

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 suppose this will result in each address having a MempoolEntryByAddress, even if sending / receving is empty. I would like to imitate the behaviour of GetUtxosByAddressess, to have some sort of standard. which i think leaves out all addresses that don't have utxo's. but i will look into it.

Copy link
Contributor Author

@D-Stacks D-Stacks Apr 26, 2022

Choose a reason for hiding this comment

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

I added a comment to reflect this discussion. The if statement will make this rpc call mimic GetUtxoEntriesByAddresses. Personally, i think all ByAddresses calls should follow the same convention / standard regarding addresses for which the call cannot find an "entry", in this case it would require the if statement, to exclude the address completely from the entries. But, if you insist I will happily oblige. Just wish to raise this point as a counter argument, before potentially changing this.

testing/integration/tx_relay_test.go Outdated Show resolved Hide resolved
@someone235 someone235 merged commit 6e2fd06 into kaspanet:dev Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants