Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

chore: use data-encoding arg to ensure data is not corrupted #806

Merged
merged 1 commit into from
Aug 14, 2018

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain commented Jul 4, 2018

Requires ipfs/js-ipfs#1420 to be merged in order to allow the test in ipfs-inactive/interface-js-ipfs-core#302 to pass.

@alanshaw
Copy link
Contributor

alanshaw commented Jul 5, 2018

Do you mean ipfs/js-ipfs#1420 not ipfs/js-ipfs#420 ?

@achingbrain
Copy link
Collaborator Author

Yes, have updated the description.

@daviddias
Copy link
Contributor

@achingbrain please rebase master onto this branch to get that CI green :)

@alanshaw alanshaw changed the title chore: use data-encoding arg to ensure data is not corrupted [WIP] chore: use data-encoding arg to ensure data is not corrupted Jul 17, 2018
@achingbrain
Copy link
Collaborator Author

This requires go-ipfs 0.4.17 to be released first - not much point merging this until then.

@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch from c1c1061 to fd74597 Compare July 17, 2018 15:15
@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch from fd74597 to 4665f52 Compare August 2, 2018 07:58
@achingbrain
Copy link
Collaborator Author

go-ipfs 0.4.17 has been released, should probably get this in so we can merge ipfs/js-ipfs#1420 and ipfs/interop#25. Needs ipfs-inactive/interface-js-ipfs-core#342 merging first it seems.

@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch from 4665f52 to 41b89f5 Compare August 2, 2018 20:56
@achingbrain
Copy link
Collaborator Author

@alanshaw CI is green, can it be merged?

@alanshaw
Copy link
Contributor

Does this break compatibility with previous go-ipfs versions?

@achingbrain
Copy link
Collaborator Author

Yes, it requires go 0.4.17.

@achingbrain
Copy link
Collaborator Author

Will this stop this from being merged?

@daviddias
Copy link
Contributor

0.4.17 has been released

@alanshaw
Copy link
Contributor

@achingbrain would you mind adding a BREAKING CHANGE message to the commit? Also, do you think it's more appropriate to prefix with "fix:" than "chore:"?

This change will mean that the API will only work with go-ipfs 0.4.17 and above and we need to make sure there's no confusion around that when it's released.

@alanshaw alanshaw changed the title [WIP] chore: use data-encoding arg to ensure data is not corrupted chore: use data-encoding arg to ensure data is not corrupted Aug 13, 2018
@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch 2 times, most recently from 201675f to 975c758 Compare August 13, 2018 17:42
@achingbrain
Copy link
Collaborator Author

Done, just trying to get Jenkins to pass.

@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch from 975c758 to 10b0120 Compare August 14, 2018 06:39
@achingbrain
Copy link
Collaborator Author

Hmm, this new test is failing, but only on Windows and only some of the time. Any ideas?

@achingbrain
Copy link
Collaborator Author

achingbrain commented Aug 14, 2018

It times out so the repo might be missing the DAG node it's trying to get and it's going to the network? It's not clear to me where that node would come from so I've added it to the repo in the test.

@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch from 4d4b889 to 97b4e8e Compare August 14, 2018 08:42
@achingbrain
Copy link
Collaborator Author

...aaaand one of the Jenkins workers is out of disk space.

@alanshaw
Copy link
Contributor

Hmm, this new test is failing, but only on Windows and only some of the time. Any ideas?

@lidel we need to somehow put the data for this hash into IPFS before we try to retrieve it.

@achingbrain
Copy link
Collaborator Author

@alanshaw I'd done that but pulled it out because it wasn't related to this PR. I've opened a separate one with my change in it here: #832

Requires go-ipfs 0.4.17 as it allows for specifying the data encoding
format when requesting object data.
@achingbrain achingbrain force-pushed the transfer-object-data-in-base64 branch from 2135f1d to 4f78609 Compare August 14, 2018 12:06
@alanshaw alanshaw merged commit 553c3fb into master Aug 14, 2018
@ghost ghost removed the in progress label Aug 14, 2018
@alanshaw alanshaw deleted the transfer-object-data-in-base64 branch August 14, 2018 22:29
args: cidB58Str
args: cidB58Str,
qs: {
'data-encoding': 'base64'
Copy link
Contributor

Choose a reason for hiding this comment

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

@alanshaw quick question: what values are allowed here?
Ones from https://github.com/multiformats/multibase/blob/master/multibase.csv ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the moment go-ipfs supports text and base64.

Go's text is the default (e.g. raw bytes), though go-ipfs doesn't escape the characters so it's http api can produce strings that are invalid in JSON and hard to replicate in JavaScript so base64 is safer.

Once ipfs/js-ipfs#1420 is in, js-ipfs will support whatever Buffer.toString([encoding[, start[, end]]]) does.

If we want to add all of the different encodings from that csv it should be pretty straightforward once both implementations support the data-encoding argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @achingbrain!

This PR reminded me of yEnc from Usenet days, so I was just looking at efficiency column in encoding standards comparison and things like Ascii85 (used in PDFs and git patch) looked interesting (due to moderate adoption and smaller overhead, eg. 25% instead of 33%).

..but then i remembered that HTTP API is often gzipped which means any gains introduced by more exotic encoding than Base64 go away.

For example: base-64 seems to compress better than base-122 🙃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants