Refactor to refmt

[dignifiedquire]

• Unmarshal uses refmt and passes existing tests
• Marshal uses refmt and passes existing tests:
  • waiting on response in panic("todo") when marshalling cbor tags polydawn/refmt#9 - fixed through not using pointers
  • waiting on response in Emptish objects fail to roundtrip cbor marshal & unmarshal polydawn/refmt#10
  • waiting on response in Nested Cbor Tags are not unmarshalled properly polydawn/refmt#13
• Cleanup

Ref #29

[whyrusleeping]

btw, you can use %x instead of %s to automatically encode to hex.

[dignifiedquire]

sweet, thank you <3

[dignifiedquire]

More tests are passing with the latest fixes + latest refmt. Some more tests to fix before we are ready.

[whyrusleeping]

I actually think that something is broken on master. In TestMarshalRoundtrip, the object that gets created (on master) looks like this:

why@manwe /t/go-ipld-cbor> echo -n a36362617a82d82a5823001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505ed82a58230012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a063666f6f636261726463617473a163717578d82a5823001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d897 | refmt cbor.hex=pretty
Map<len:3> {
        "baz": Array<len:2> [
                _tag:42_ 001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e
                _tag:42_ 0012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a0
        ]
        "foo": "bar"
        "cats": Map<len:1> {
                "qux": _tag:42_ 001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d897
        }
}

The string 'hello' is missing. This test started failing once we switched to refmt, and the created object looks like:

why@manwe ~/g/s/g/i/go-ipld-cbor> echo -n a46362617a82d82a5823001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505ed82a58230012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a063666f6f636261726463617473a163717578d82a5823001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d8976568656c6c6fd82a5823001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e | refmt cbor.hex=pretty
Map<len:4> {
        "baz": Array<len:2> [
                _tag:42_ 001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e
                _tag:42_ 0012205773487d221545d26fd0f57fdb3a7d986bc479a850d7b0d762e8c7f4772790a0
        ]
        "foo": "bar"
        "cats": Map<len:1> {
                "qux": _tag:42_ 001220434be0538b88f385bdb9d62de34e296a0908c857a876aec09e1f52673596d897
        }
        "hello": _tag:42_ 001220aed1fe98cda4ba5a1681a19aa768f73b9d707c5621c7effdf2938e242080505e
}

So while I was trying to avoid changing any tests, I think we actually should here.

[whyrusleeping]

Ignore my previous comment, it looks like @dignifiedquire added that hello key in earlier commits but didnt update the expected hash.

[whyrusleeping]

All thats left here is to add in the right sorting for maps to refmt for canonical cbor. I can manually make that change (and have locally), but would rather do it in a way thats flexible and have it officially be a part of refmt.

[dignifiedquire]

yeah that was probably some debugging tries that I forgot to clean up

…

On 13. Dec 2017, 23:38 +0100, Whyrusleeping ***@***.***>, wrote: All thats left here is to add in the right sorting for maps to refmt for canonical cbor. I can manually make that change (and have locally), but would rather do it in a way thats flexible and have it officially be a part of refmt. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dignifiedquire]

@whyrusleeping I think this is ready as soon as polydawn/refmt#19 is merged and we have a gx version of refmt.

[whyrusleeping]

Alright, we're ready to go here I think

[dignifiedquire]

@whyrusleeping is there a reason this is not merged yet?

[whyrusleeping]

@dignifiedquire mostly because we need to do the update into go-ipfs intentionally, and not have it get included arbitrarily with a bunch of other updates. This sort of thing is a higher risk change, and is something we want to test carefully and individually.

[Stebalien]

This is blocked until it stops deserializing CIDs as *cid.Cid cid.Cid and starts deserializing them to *cid.Cid.

[whyrusleeping]

@Stebalien cant we just change the castBytesToCid method to return a *cid.Cid?

[Stebalien]

@whyrusleeping no, this is a limitation of refmt as it stands (I've discussed it with @warpfork and I just filed an issue (polydawn/refmt#32) so we can track it).

We could fix all the CIDs up using traverse but that would mean an additional step.

---

My other open concern is that I'm not sure we really want to expose the RegisterCBORType function as it could screw with Resolve (maybe?).

[Kubuxu]

We also need: polydawn/refmt@2e9ba99

[warpfork]

I have attempted to unblock :)

[Stebalien]

I've just rebased this on master in the refmt-rebased branch. Basically, we now just use cid.Cid (no pointers) so I got rid of all the "if we have a pointer do x, if we don't do y" stuff.

I've done a sanity check with the new uber-cool git range-diff master refmt refmt-rebased command but feel free to do the same.

@dignifiedquire please reset this branch to refmt-rebased when you're ready.

[warpfork]

What's the landing strategy going to be for this? IIUC, there's already releases of go-ipld-cbor cut from this branch being used in some other projects...

I guess gx snapshots also do preserve those, but ditching the original git commit hashes matching those releases would also seem pretty lossy.

Idea: do a giant git checkout master && git merge -s ours refmt to keep this history (but not apply the diff to master yet -- so there's no conflict resolution to do) and make it referenced from master so we can drop the branch ref... and then do another giant commit that's the rebased squash of this (with conflicts resolved).

Maybe that's unnecessary footwork, just floating the idea.

[Stebalien]

It's probably fine to drop the release commits, IMO. That shouldn't actually break anything due to how gx works.

[dignifiedquire]

@Stebalien maybe best you do what you want with the branch, I have a fork for the current version I need, so nothing should break.

…

On 17. Sep 2018, 20:13 +0200, Steven Allen ***@***.***>, wrote: It's probably fine to drop the release commits, IMO. That shouldn't actually break anything due to how gx works. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.