btcsuite / btcd

An alternative full node bitcoin implementation written in Go (golang)
https://github.com/btcsuite/btcd/blob/master/README.md
ISC License
6.11k stars 2.32k forks source link

chainhash: JSON Unmarshal hash from appropriate string. #1952

Closed LindenWang01 closed 1 year ago

LindenWang01 commented 1 year ago

This commit(btcsuite@1d6e578) occurred a problem: "error": "json: cannot unmarshal string into Go struct field OutPoint.TxIn.PreviousOutPoint.Hash of type chainhash.Hash"

adds a UnmarshalJSON() method to chainhash.Hash to fix it.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4258913166


Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 3 86.27%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 4163994991: 0.006%
Covered Lines: 26641
Relevant Lines: 48192

💛 - Coveralls
kcalvinalvin commented 1 year ago

How about a simple test in chaincfg/chainhash/hash_test.go to catch these bugs in the future?

Maybe something like:

func TestHashJsonMarshal(t *testing.T) {
        tests := []struct {
                hash Hash
                err  error
        }{
                // Genesis hash.
                {
                        mainNetGenesisHash,
                        nil,
                },
        }

        unexpectedErrStr := "TestHashJsonMarshal #%d got an unexpected error: %v"
        unexpectedResultStr := "TestHashJsonMarshal #%d got: %v want: %v"
        for i, test := range tests {
                bytes, err := test.hash.MarshalJSON()
                if err != nil {
                        t.Errorf(unexpectedErrStr, i, err)
                        continue
                }

                got := new(Hash)
                err = json.Unmarshal(bytes, got)
                if err != nil {
                        t.Errorf(unexpectedErrStr, i, err)
                        continue
                }

                if !test.hash.IsEqual(got) {
                        t.Errorf(unexpectedResultStr, i, got, &test.hash)
                        continue
                }
        }
}
Roasbeef commented 1 year ago

Approved the CI run.

kcalvinalvin commented 1 year ago

I see no reason for the holdup for this PR. Anything else to address? @jcvernaleo @Roasbeef

jcvernaleo commented 1 year ago

@kcalvinalvin I have no idea why I approved but didn't merge. I must have gotten distracted or something. That delay's on me, sorry about that. Merging now.

chappjc commented 1 year ago

MarshalJSON was in chaincfg/chainhash/v1.0.2. If possible, it would be good to tag a v1.0.3 with the matching UnmarshalJSON method added in this PR.