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

Show hash on sendrawtransaction #796

Merged
merged 9 commits into from Jun 4, 2019

Conversation

4 participants
@shargon
Copy link
Member

commented Jun 4, 2019

Close #795

@codecov-io

This comment has been minimized.

Copy link

commented Jun 4, 2019

Codecov Report

Merging #796 into master will decrease coverage by 0.15%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
- Coverage   37.75%   37.59%   -0.16%     
==========================================
  Files         176      176              
  Lines       12439    12442       +3     
==========================================
- Hits         4696     4678      -18     
- Misses       7743     7764      +21
Impacted Files Coverage Δ
neo/Network/RPC/RpcServer.cs 0% <0%> (ø) ⬆️
neo/Network/P2P/Payloads/Transaction.cs 56.83% <100%> (ø) ⬆️
neo/IO/Actors/PriorityMessageQueue.cs 62.06% <0%> (-24.14%) ⬇️
neo/IO/Actors/PriorityMailbox.cs 75% <0%> (-12.5%) ⬇️
neo/Network/P2P/TaskManager.cs 11.17% <0%> (-5.59%) ⬇️

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 a8ada56...2c25f48. Read the comment docs.

var ret = new JObject();
ret["result"] = true;
ret["hash"] = hash.ToString();
return ret;

This comment has been minimized.

Copy link
@ixje

ixje Jun 4, 2019

Contributor

I can't quickly tell but will this result in

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": true,
  "hash": "some tx id hash"
}

or in

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
     "result": true,
     "hash": "some tx id hash"
   }
}

?

This comment has been minimized.

Copy link
@shargon

shargon Jun 4, 2019

Author Member

The second one, but i will change result for 'relayed'

{
var ret = new JObject();
ret["relayed"] = true;
ret["hash"] = hash.ToString();

This comment has been minimized.

Copy link
@ixje

ixje Jun 4, 2019

Contributor

I think we should call this txid instead of hash to stay consistent. e.g. sendtoaddress and sendfrom also use txid

Otherwise looks good

This comment has been minimized.

Copy link
@erikzhang

erikzhang Jun 4, 2019

Member

It's weird to return two different structures for blocks and transactions.

This comment has been minimized.

Copy link
@shargon

shargon Jun 4, 2019

Author Member

Maybe we can leave blocks as before, but looks weird too

This comment has been minimized.

Copy link
@erikzhang

erikzhang Jun 4, 2019

Member

Maybe we should use hash uniformly.

This comment has been minimized.

Copy link
@shargon

shargon Jun 4, 2019

Author Member

agree

This comment has been minimized.

Copy link
@shargon

shargon Jun 4, 2019

Author Member

We can change the neo-plugin to "hash"

Show resolved Hide resolved neo/Network/RPC/RpcServer.cs Outdated
@erikzhang

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Test failed.

@shargon shargon merged commit 918d1e4 into neo-project:master Jun 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shargon shargon deleted the shargon:hash-on-send branch Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.