Skip to content

Show hash on sendrawtransaction#796

Merged
shargon merged 9 commits intoneo-project:masterfrom
shargon:hash-on-send
Jun 4, 2019
Merged

Show hash on sendrawtransaction#796
shargon merged 9 commits intoneo-project:masterfrom
shargon:hash-on-send

Conversation

@shargon
Copy link
Copy Markdown
Member

@shargon shargon commented Jun 4, 2019

Close #795

@shargon shargon mentioned this pull request Jun 4, 2019
Closed
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 4, 2019 •
edited
Loading

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.

ixje
ixje reviewed Jun 4, 2019
View reviewed changes
neo/Network/RPC/RpcServer.cs
var ret = new JObject();
ret["result"] = true;
ret["hash"] = hash.ToString();
return ret;
Copy link
Copy Markdown
Contributor

@ixje ixje Jun 4, 2019

Choose a reason for hiding this comment

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

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"
   }
}

?

Copy link
Copy Markdown
Member Author

@shargon shargon Jun 4, 2019

Choose a reason for hiding this comment

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

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

ixje
ixje suggested changes Jun 4, 2019
View reviewed changes
neo/Network/RPC/RpcServer.cs
{
var ret = new JObject();
ret["relayed"] = true;
ret["hash"] = hash.ToString();
Copy link
Copy Markdown
Contributor

@ixje ixje Jun 4, 2019

Choose a reason for hiding this comment

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

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

Otherwise looks good

Copy link
Copy Markdown
Member

@erikzhang erikzhang Jun 4, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@shargon shargon Jun 4, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@erikzhang erikzhang Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we should use hash uniformly.

Copy link
Copy Markdown
Member Author

@shargon shargon Jun 4, 2019

Choose a reason for hiding this comment

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

agree

Copy link
Copy Markdown
Member Author

@shargon shargon Jun 4, 2019

Choose a reason for hiding this comment

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

We can change the neo-plugin to "hash"

ixje
ixje previously approved these changes Jun 4, 2019
View reviewed changes
erikzhang
erikzhang reviewed Jun 4, 2019
View reviewed changes
erikzhang
erikzhang previously approved these changes Jun 4, 2019
View reviewed changes
@erikzhang
Copy link
Copy Markdown
Member

erikzhang commented Jun 4, 2019

Test failed.

@shargon shargon merged commit 918d1e4 into neo-project:master Jun 4, 2019
@shargon shargon deleted the hash-on-send branch June 4, 2019 21:00
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
@Celia18305 @superboyiii
add link in toc (neo-project#796)
16af40a
Tommo-L pushed a commit to Tommo-L/neo that referenced this pull request Jun 22, 2020
@shargon @Tommo-L
Show hash on sendrawtransaction (neo-project#796)
77363cc 
* Show hash on sendrawtransaction

* Update RpcServer.cs

* txid

* relayed

* Revert to hash

* txid -> hash

* Update RpcServer.cs

* Fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erikzhang erikzhang erikzhang approved these changes

+1 more reviewer

@ixje ixje ixje left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

use sendRawTranscation , how to get txid??

4 participants

@shargon @codecov-io @erikzhang @ixje