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

feat: add 'ipfs multibase' commands #8180

Merged
merged 9 commits into from Aug 18, 2021

Conversation

coryschwartz
Copy link

@coryschwartz coryschwartz commented Jun 8, 2021

This is intended to implement part 1 of #7939 (comment)

Usage examples

See #8180 (comment)

@coryschwartz coryschwartz marked this pull request as ready for review June 9, 2021 23:55
@coryschwartz coryschwartz linked an issue Jun 10, 2021 that may be closed by this pull request
@aschmahmann aschmahmann requested a review from lidel June 14, 2021 15:33
@ipfs ipfs deleted a comment from welcome bot Jun 15, 2021
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @coryschwartz, looks good, some cosmetic asks in comments inline.

Before we merge this PR, it needs tests similar to test/sharness/t0290-cid.sh.

Tests should cover:

  • encoding and decoding round (echo foo | ipfs multibase encode -b base32 | ipfs multibase decode and expect "foo" back)
    • for a file
    • for stdin and stdout
  • error behaviors when decoder finds
    • unknown base prefix
    • character outside expected base dictionary

core/commands/multibase.go Outdated Show resolved Hide resolved
core/commands/root.go Outdated Show resolved Hide resolved
core/commands/multibase.go Outdated Show resolved Hide resolved
@lidel lidel added this to In Review in Maintenance Priorities - Go via automation Jun 25, 2021
@lidel lidel added this to the go-ipfs 0.10 milestone Jun 30, 2021
@BigLep BigLep added this to In Review in Go IPFS Roadmap via automation Jul 1, 2021
@BigLep BigLep removed this from In Review in Go IPFS Roadmap Jul 1, 2021
@lidel lidel added this to In Review in Go IPFS Roadmap via automation Jul 1, 2021
@lidel lidel moved this from In Review to In Progress in Go IPFS Roadmap Jul 1, 2021
@lidel lidel moved this from In Review to In Progress in Maintenance Priorities - Go Jul 1, 2021
@lidel lidel changed the title add multibase helper command feat: add 'ipfs multibase' commands Jul 1, 2021
@lidel
Copy link
Member

lidel commented Jul 1, 2021

Cleaned up and added tests.

@ribasushi @Stebalien @aschmahmann lmk if we can improve CLI ergonomics/docs here in any way for ipfs multibase commands.

Usage examples for quick eyeballing:

ipfs multibase

> ipfs multibase  --help
USAGE
  ipfs multibase - Encode and decode files or stdin with multibase format

SYNOPSIS
  ipfs multibase

SUBCOMMANDS
  ipfs multibase decode <encoded_file> - Decode multibase string
  ipfs multibase encode <file>         - Encode data into multibase string
  ipfs multibase list                  - List available multibase encodings.

ipfs multibase decode

> ipfs multibase decode --help
USAGE
  ipfs multibase decode <encoded_file> - Decode multibase string

SYNOPSIS
  ipfs multibase decode [--] <encoded_file>

ARGUMENTS

  <encoded_file> - encoded data to decode

DESCRIPTION

  This command expects multibase inside of a file or via stdin:

    > echo -n hello | ipfs multibase encode -b base16 > file
    > cat file
    f68656c6c6f

    > ipfs multibase decode file
    hello

    > cat file | ipfs multibase decode
    hello

ipfs multibase encode

> ipfs multibase encode --help
USAGE
  ipfs multibase encode <file> - Encode data into multibase string

SYNOPSIS
  ipfs multibase encode [-b=<b>] [--] <file>

ARGUMENTS

  <file> - data to encode

OPTIONS

  -b  string - multibase encoding. Default: base64url.

DESCRIPTION

  This command expects a file name or data provided via stdin.

  By default it will use URL-safe base64url encoding,
  but one can customize used base with -b:

    > echo hello | ipfs multibase encode -b base16 > output_file
    > cat output_file
    f68656c6c6f0a

    > echo hello > input_file
    > ipfs multibase encode -b base16 input_file
    f68656c6c6f0a

ipfs multibase list

This is alias/same command as pre-existing ipfs cid bases.
Decided to add it here as well for completeness and better UX.

> ipfs multibase list --help
USAGE
  ipfs multibase list - List available multibase encodings.

SYNOPSIS
  ipfs multibase list [--prefix] [--numeric]

OPTIONS

  --prefix   bool - also include the single letter prefixes in addition to the code.
  --numeric  bool - also include numeric codes.
> ipfs multibase list --prefix --numeric
      0  identity
0    48  base2
b    98  base32
B    66  base32upper
c    99  base32pad
C    67  base32padupper
f   102  base16
F    70  base16upper
k   107  base36
K    75  base36upper
m   109  base64
M    77  base64pad
t   116  base32hexpad
T    84  base32hexpadupper
u   117  base64url
U    85  base64urlpad
v   118  base32hex
V    86  base32hexupper
z   122  base58btc
Z    90  base58flickr

@lidel lidel moved this from In Progress to In Review in Go IPFS Roadmap Jul 1, 2021
@lidel lidel moved this from In Progress to In Review in Maintenance Priorities - Go Jul 1, 2021
@lidel lidel self-requested a review July 1, 2021 23:51
@aschmahmann aschmahmann self-requested a review July 9, 2021 15:11
@lidel
Copy link
Member

lidel commented Jul 15, 2021

  • Note from sync discussion:
    • ok to merge as-is
    • nice to have: add -o for outputing to file without piping (better UX in Powershell). Could be this PR or separate one.

@aschmahmann
Copy link
Contributor

aschmahmann commented Jul 15, 2021

@lidel another thing we may want to add here is a transcode function so that you don't have to do ipfs multibase decode | ipfs multibase encode -b=base16, but can instead do ipfs multibase transcode -b=base16. Not having this makes life pretty painful for environments that don't tolerate binary outputs well (e.g. PowerShell, but I suspect there are others)

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Please consider my comment. I think the correct way to do this is to have a stream decoder, instead of reading the entirety of the (possibly huge) file into memory.

if err != nil {
return fmt.Errorf("failed to access file: %w", err)
}
encoded_data, err := ioutil.ReadAll(file)
Copy link
Contributor

@gammazero gammazero Jul 16, 2021

Choose a reason for hiding this comment

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

Would it be worth implementing a multibase Decoder that would take an io.Reader and return one as well, with a signature like NewDecoder(io.Reader) io.Reader

Then this could all be done without having to read the entire file into memory.

decReader := mbase.NewDecoder(file)
return resp.emit(decReader)

@lidel Seems like this is something that should exist, or am I missing an obvious reason it does not? Seems like a NewDecode function could start reading the stream, and then construct the correct decoder and return it as an io.Reader.

Copy link
Member

Choose a reason for hiding this comment

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

Good question! I filled upstream issue multiformats/go-multibase#44 to discuss this, but out of scope for this PR.

@Stebalien
Copy link
Member

Are any of the requested changes here blocking or just improvements? If they're just improvements, can we merge and iterate?

@gammazero
Copy link
Contributor

Not blocking. Only asking implementor to take note before merging.

@lidel
Copy link
Member

lidel commented Aug 11, 2021

For the sake of shipping basic ipfs multibase encode|decode in go-ipfs 0.10,
I've extracted nice-to-haves to separate issues that can be tackled separately:

@aschmahmann this PR is ready for your final 👀 and a squash-merge.

@BigLep BigLep removed this from In Review in Maintenance Priorities - Go Aug 13, 2021
@BigLep BigLep removed this from the go-ipfs 0.10 milestone Aug 14, 2021
"decode": mbaseDecodeCmd,
"list": basesCmd,
},
Extra: CreateCmdExtras(SetDoesNotUseRepo(true)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Going to merge for now as this seems like it wouldn't hurt and matches what ipfs cid does.

However, I don't think this really works properly without applying the flag to each subcommand.

@lidel let's revisit during the RC process what we want to do here and figure out if ipfs cid has similar problems.

Copy link
Member

Choose a reason for hiding this comment

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

Continued in #8375 (comment)

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

Successfully merging this pull request may close these issues.

Pubsub subscription data encoding
6 participants