Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Make getTransactionStatus argument format somewhat intuitive #338

Merged
merged 2 commits into from
Aug 18, 2021
Merged

Make getTransactionStatus argument format somewhat intuitive #338

merged 2 commits into from
Aug 18, 2021

Conversation

bogdan
Copy link
Contributor

@bogdan bogdan commented Aug 12, 2021

Zilliqa JSON RPC is inconsistent. No one would ever guess that getTransactionStatus method suppose to accept txHash argument without 0x prefix. Made it indifferent to the prefix.

@bogdan bogdan requested a review from bb111189 as a code owner August 12, 2021 13:22
@bogdan bogdan requested a review from a user August 12, 2021 13:22
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2021

Codecov Report

Merging #338 (7e7b76c) into dev (c054901) will increase coverage by 0.08%.
The diff coverage is 72.72%.

❗ Current head 7e7b76c differs from pull request most recent head 399b1b5. Consider uploading reports for the commit 399b1b5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #338      +/-   ##
==========================================
+ Coverage   80.58%   80.67%   +0.08%     
==========================================
  Files          44       44              
  Lines        1787     1790       +3     
  Branches      299      298       -1     
==========================================
+ Hits         1440     1444       +4     
+ Misses        346      345       -1     
  Partials        1        1              
Impacted Files Coverage Δ
...kages/zilliqa-js-account/src/transactionFactory.ts 88.88% <0.00%> (ø)
packages/zilliqa-js-core/src/net.ts 98.59% <ø> (ø)
packages/zilliqa-js-crypto/src/random.ts 75.00% <61.53%> (+12.50%) ⬆️
packages/zilliqa-js-account/src/transaction.ts 90.20% <100.00%> (ø)
packages/zilliqa-js-account/src/wallet.ts 82.03% <100.00%> (-0.14%) ⬇️
packages/zilliqa-js-blockchain/src/chain.ts 62.92% <100.00%> (ø)
packages/zilliqa-js-crypto/src/util.ts 82.45% <100.00%> (ø)
packages/zilliqa-js-subscriptions/src/ws.ts 64.96% <100.00%> (ø)
packages/zilliqa-js-util/src/unit.ts 80.95% <100.00%> (ø)

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 c054901...399b1b5. Read the comment docs.

@bb111189
Copy link
Contributor

bb111189 commented Aug 16, 2021

Zilliqa JSON RPC is inconsistent. No one would ever guess that getTransactionStatus method suppose to accept txHash argument without 0x prefix. Made it indifferent to the prefix.

Hey! Just wondering where you spot the inconsistencies for the tx hash. I look through the API and couldn't find it. Might be something we can fix

@bogdan
Copy link
Contributor Author

bogdan commented Aug 16, 2021

Hey, sorry for confusing you. It is truly consistent.

In our case, the inconsistency came from the fact that we manage ethereum and zilliqa transactions with the same code and ethereum has the 0x convention and zilliqa does not.

@bb111189
Copy link
Contributor

bb111189 commented Aug 17, 2021

Hey, sorry for confusing you. It is truly consistent.

In our case, the inconsistency came from the fact that we manage ethereum and zilliqa transactions with the same code and ethereum has the 0x convention and zilliqa does not.

Maybe for consistency between different API, can you help me add the same replace 0x logic to getTransaction(). Thanks alot

@bb111189 bb111189 merged commit c8bc398 into Zilliqa:dev Aug 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants