chore: use data-encoding arg to ensure data is not corrupted #806
Conversation
ghost
assigned 
|
Do you mean ipfs/js-ipfs#1420 not ipfs/js-ipfs#420 ? |
|
Yes, have updated the description. |
|
@achingbrain please rebase master onto this branch to get that CI green :) |
alanshaw
changed the title
|
This requires go-ipfs 0.4.17 to be released first - not much point merging this until then. |
achingbrain
force-pushed
the
transfer-object-data-in-base64
branch
from
c1c1061
to
fd74597
Compare
achingbrain
force-pushed
the
transfer-object-data-in-base64
branch
from
fd74597
to
4665f52
Compare
|
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
force-pushed
the
transfer-object-data-in-base64
branch
from
4665f52
to
41b89f5
Compare
|
@alanshaw CI is green, can it be merged? |
|
Does this break compatibility with previous go-ipfs versions? |
|
Yes, it requires go 0.4.17. |
|
Will this stop this from being merged? |
|
0.4.17 has been released |
|
@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
changed the title
achingbrain
force-pushed
the
transfer-object-data-in-base64
branch
2 times, most recently
from
201675f
to
975c758
Compare
|
Done, just trying to get Jenkins to pass. |
achingbrain
force-pushed
the
transfer-object-data-in-base64
branch
from
975c758
to
10b0120
Compare
|
Hmm, this new test is failing, but only on Windows and only some of the time. Any ideas? |
|
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
force-pushed
the
transfer-object-data-in-base64
branch
from
4d4b889
to
97b4e8e
Compare
|
...aaaand one of the Jenkins workers is out of disk space. |
achingbrain
force-pushed
the
transfer-object-data-in-base64
branch
from
97b4e8e
to
2135f1d
Compare
@lidel we need to somehow put the data for this hash into IPFS before we try to retrieve it. |
Requires go-ipfs 0.4.17 as it allows for specifying the data encoding format when requesting object data.
achingbrain
force-pushed
the
transfer-object-data-in-base64
branch
from
2135f1d
to
4f78609
Compare
| args: cidB58Str | ||
| args: cidB58Str, | ||
| qs: { | ||
| 'data-encoding': 'base64' |
There was a problem hiding this comment.
@alanshaw quick question: what values are allowed here?
Ones from https://github.com/multiformats/multibase/blob/master/multibase.csv ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙃
Requires ipfs/js-ipfs#1420 to be merged in order to allow the test in ipfs-inactive/interface-js-ipfs-core#302 to pass.