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

Wrong cbor version used in tests #266

Closed
dignifiedquire opened this issue Apr 6, 2018 · 8 comments
Closed

Wrong cbor version used in tests #266

dignifiedquire opened this issue Apr 6, 2018 · 8 comments
Labels
C-bug Category: This is a bug

Comments

@dignifiedquire
Copy link
Contributor

The gx version for go-ipld-cbor used in commands/dag_daemon_test.go is not the one used in package.json. Updating it breaks the test, so this needs some more detailed look into.

@dignifiedquire dignifiedquire added C-bug Category: This is a bug c5 labels Apr 6, 2018
@dignifiedquire dignifiedquire added this to the Sprint 4 milestone Apr 6, 2018
@laser
Copy link
Contributor

laser commented Apr 6, 2018

@dignifiedquire

There was a discussion in #c-filecoin-dev about this issue earlier in the week:

https://protocollabs.slack.com/archives/G7XUR2TU2/p1522789325000360

A quick recap:

The DAG service returns a value X of type Node, which is defined in version 1.2.12 (QmNRz7BDWfdFNVLt7AVvmRefkrURD25EeoipcXqo6yoXU1) of go-ipld-cbor, which I emit in a command. If the API consumer asks for JSON, this will result in JSON showing up in the terminal or in an HTTP response.

If I then try to deserialize that JSON back into an IPLD Node Y using the version of go-ipld-cbor which have listed in our package.json (1.3.1, QmRVSCwQtW1rjHCay9NqKXDwbtKTgDcN4iY7PrpSqfKM5D), the CIDs of X and Y do not match.

The workaround that I came up with involved using, in the test, the version of go-ipld-cbor which was used when creating the JSON representation of X (1.2.12, not 1.3.1).

@frrist investigated updating go-ipfs such that it used the same version of go-ipld-cbor that go-filecoin uses (1.3.1). You said that "go-ipld-cbor 1.3 changes the underlying cbor impl and was not ready to be used in ipfs last time I spoke with why," so I believe that effort has stalled, for now.

@dignifiedquire - Given the above, do you have an alternative proposal for how I can verify that the output of dag get is correct (in the daemon test)?

@dignifiedquire dignifiedquire removed this from the Sprint 4 milestone Apr 7, 2018
@dignifiedquire
Copy link
Contributor Author

Alright, removing this from the current sprint, but open until we fix the issue upstream in go-ipfs

@dignifiedquire dignifiedquire removed the c5 label May 25, 2018
@mishmosh
Copy link
Contributor

mishmosh commented Jun 13, 2018

@dignifiedquire or @laser Can you add a link to the relevant go-ipld-cbor issue (or create & link if it doesn't exist yet)?

@travisperson
Copy link
Contributor

This might be loosely related to an issue in refmt: polydawn/refmt#12

When updating the go-ipld-cbor version from dag_daemon_test.go to match the main version from package.json, the following error is returned when running cbor.DecodeInto

unmarshal error: cannot assign <f:0> to uint8 field

MessageReceipt is the only type inside of Block that appears to have a uint8 type, so it might be related to that. The MessageReceipt are nil / null though, so I would suspect that they would be ignored.

@travisperson
Copy link
Contributor

Is go-ipld-cbor being used on purpose in this test for any reason? It seems that it can just be dropped, and call json.Unmarshal directly to a Block. Going from JSON directly to CBOR IPLD has issues because numbers are represented as float64 values in JSON. These then get encoded in the CBOR and make decoding into other structures pretty difficult.

@phritz
Copy link
Contributor

phritz commented Jul 26, 2018

Note: go-ipfs moving to our version of cbor here: ipfs/go-ipld-cbor#29

@dignifiedquire
Copy link
Contributor Author

The move has happened, we can update all the things, and drop our own fork

@anorth
Copy link
Member

anorth commented Jul 22, 2019

@dignifiedquire @travisperson is this issue obsolete?

@anorth anorth closed this as completed Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

6 participants