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

refactor(core-p2p): remove transactionsFromIds endpoint #1911

Merged
merged 11 commits into from
Dec 27, 2018
Merged

refactor(core-p2p): remove transactionsFromIds endpoint #1911

merged 11 commits into from
Dec 27, 2018

Conversation

alessiodf
Copy link
Contributor

@alessiodf alessiodf commented Dec 25, 2018

Proposed changes

Fixes #1910 by sanitising the tx ids and applies v1 compatibility where needed removing the transactionsFromIds endpoint as it is no longer required

Types of changes

  • Refactor

Checklist

Further comments

I'm not sure if this will have any implications if a block is received with headers only, since it is untested. After talking with @supaiku0 it seems header-only blocks aren't used in v2 anymore so the whole endpoint is no longer needed.

* fix: apply v1 compatibility if the transaction version is 1; was previously not returning any results for v1 transactions due to missing `id`
* fix: regex now accepts only exactly 64 hex chars per id (as tx id length is 64 chars); was previously allowing any 32+ character value including non-hex characters
* fix: return `success: false` if no valid ids were passed; was previously returning the raw query syntax error from the db in the response
* fix: change `row.block_id` to `row.blockId` so the block id for each tx is returned; previously block id was undefined and omitted from the response
@alessiodf alessiodf changed the title Get transactions from ids (core-p2p) fix: make transactionsFromIds endpoint work with v1 transactions Dec 25, 2018
@alessiodf alessiodf changed the title (core-p2p) fix: make transactionsFromIds endpoint work with v1 transactions fix(core-p2p): make transactionsFromIds endpoint work with v1 transactions Dec 25, 2018
@codecov-io
Copy link

codecov-io commented Dec 25, 2018

Codecov Report

Merging #1911 into develop will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1911      +/-   ##
===========================================
+ Coverage    39.38%   39.55%   +0.17%     
===========================================
  Files          350      350              
  Lines         7658     7624      -34     
  Branches      1166     1130      -36     
===========================================
  Hits          3016     3016              
+ Misses        4628     4595      -33     
+ Partials        14       13       -1
Impacted Files Coverage Δ
...ackages/core-p2p/src/server/versions/1/handlers.ts 0% <ø> (ø) ⬆️
packages/core-p2p/src/server/versions/1/schema.ts 0% <ø> (ø) ⬆️
packages/core-p2p/src/peer.ts 0% <ø> (ø) ⬆️
packages/core-logger-winston/src/formatter.ts 0% <0%> (ø) ⬆️

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 f8c4796...9fec4de. Read the comment docs.

@alessiodf alessiodf changed the title fix(core-p2p): make transactionsFromIds endpoint work with v1 transactions refactor(core-p2p): remove transactionsFromIds endpoint Dec 27, 2018
@faustbrian faustbrian requested a review from spkjp December 27, 2018 08:03
@faustbrian faustbrian merged commit 9900caa into ArkEcosystem:develop Dec 27, 2018
@alessiodf alessiodf deleted the getTransactionsFromIds branch December 27, 2018 16:45
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.

4 participants