Skip to content

Various RPC fixes#1511

Merged
ebuchman merged 6 commits intodevelopfrom
bucky/rpc-fixes
Apr 27, 2018
Merged

Various RPC fixes#1511
ebuchman merged 6 commits intodevelopfrom
bucky/rpc-fixes

Conversation

@ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Apr 27, 2018

n.addrBook.AddOurAddress(nodeInfo.NetAddress())

// Run the RPC server first
// so we can eg. receive txs for the first block
Copy link
Contributor

Choose a reason for hiding this comment

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

first in relation to what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to the p2p server we start after


func (ps *PartSet) MarshalJSON() ([]byte, error) {
if ps == nil {
return []byte("nil-PartSet"), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be a valid json? like null? or maybe []byte("\"nil-PartSet\"")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it should. maybe we should just do {}

defer ps.mtx.Unlock()

return cdc.MarshalJSON(struct {
CountTotal string `json:"count/total"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make them 2 separate fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured there's no need to take up the extra space but maybe that's silly (dump consensus is a big dump already). Also we're typically more interested in the ratio than we are in the two numbers

// and without the height/round/type_ (since its already included in the votes).
func (voteSet *VoteSet) MarshalJSON() ([]byte, error) {
voteStrings := make([]string, len(voteSet.votes))
for i, vote := range voteSet.votes {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about

voteSet.mtx.Lock()
defer voteSet.mtx.Unlock()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably right. String() doesn't have it tho

PartsBitArray *cmn.BitArray `json:"parts_bit_array"`
}{
fmt.Sprintf("%d/%d", ps.Count(), ps.Total()),
ps.partsBitArray.Copy(),
Copy link
Contributor

Choose a reason for hiding this comment

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

now that we have custom marshaller for BitArray's, I think we can drop .Copy() https://github.com/tendermint/tmlibs/blob/develop/common/bit_array.go#L327

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

@ebuchman
Copy link
Contributor Author

Thanks for the review @melekes !

@codecov-io
Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #1511 into develop will increase coverage by 0.17%.
The diff coverage is 60%.

@@             Coverage Diff             @@
##           develop    #1511      +/-   ##
===========================================
+ Coverage    63.67%   63.84%   +0.17%     
===========================================
  Files          102      102              
  Lines         9461     9457       -4     
===========================================
+ Hits          6024     6038      +14     
+ Misses        2873     2860      -13     
+ Partials       564      559       -5
Impacted Files Coverage Δ
p2p/node_info.go 50% <ø> (ø) ⬆️
node/node.go 64.63% <60%> (ø) ⬆️
consensus/ticker.go 90.9% <0%> (-4.93%) ⬇️
rpc/client/httpclient.go 68.36% <0%> (-1.03%) ⬇️
p2p/peer.go 63.76% <0%> (ø) ⬆️
consensus/state.go 78.01% <0%> (+1.22%) ⬆️
consensus/reactor.go 74.04% <0%> (+1.71%) ⬆️

@ebuchman
Copy link
Contributor Author

@melekes addressed your comments. PTAL and approve if you like :)

@ebuchman ebuchman merged commit 1188dfe into develop Apr 27, 2018
@ebuchman ebuchman deleted the bucky/rpc-fixes branch April 27, 2018 15:45
firelizzard18 pushed a commit to AccumulateNetwork/tendermint that referenced this pull request Feb 1, 2024
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.

3 participants