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

fix: add isCryptographicIPNS check for ipns-ns #5

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

lidel
Copy link

@lidel lidel commented Feb 11, 2021

@makoto @Arachnid
This is a PoC that makes migration away from non-cryptographic identifiers in ipns-ns (ensdomains/ens-app#849 (comment) less painful:

  • decoding legacy non-cryptographic values in ipns-ns + printing deprecation warning
  • not allowing new ones to be encoded

👉 this PR as a starting point for a discussion, do not merge it.

  • I don't like how hacky detection of inlined names is, but this is the only way to correctly decode string like app.uniswap.org in your GUI web app and at the same time refuse to encode a non-libp2p-key string inlined inside Base58 multihash.

  • I am not sure if we should pollute this library with workarounds for things that should be either in web interface at ens-app or in the at the contract level (if ENS did CID validation at the contract level, it would never allow names in the first place).

    • ❓ namely, this library throws on strings app.uniswap.org, so it receives app.uniswap.org already encoded as an identity multihash with value inlined as digest – where is this happening?
      • I suspect that if ens-app passed original identifier as-is, and did not turn it to Base58, then this entire PR would not be necessary.

lmk your thought on this

This is a poc for enabling support for legacy non-cryptographic values
while not allowing new ones to be encoded.
@makoto
Copy link
Member

makoto commented Feb 15, 2021

Hi, @lidel . I tried test integration on app-ens and worked fine.

I am not sure if we should pollute this library with workarounds for things

Given this library seems used at other apps like Metamask, probably better to keep it here.

I suspect that if ens-app passed original identifier as-is, and did not turn it to Base58, then this entire PR would not be necessary

True, though given uniswap already used the old code, I assume it would fail without it?

I could merge this after tidying up a few indentations, etc.

Replaces duplicated code with a single reusable check
@lidel
Copy link
Author

lidel commented Feb 15, 2021

Given this library seems used at other apps like Metamask, probably better to keep it here.

Fair point. In that case let's fix this here.
I cleaned this up in adff4af, should be easier to audit and maintain.

@makoto Feel free to review / tweak as needed.

True, though given uniswap already used the old code, I assume it would fail without it?

I sadly have no bandwidth for testing that, but possible.
So I guess this PR is our best bet to handle this in backward-compatible fashion.

@lidel lidel changed the title [do not merge] decode dnslink in ipns-ns, but throw on encode fix: add isCryptographicIPNS check for ipns-ns Feb 15, 2021
@lidel lidel marked this pull request as ready for review February 15, 2021 19:35
@makoto makoto merged commit eb824b8 into ensdomains:master Feb 15, 2021
@lidel lidel deleted the test/dnslink-handling branch February 15, 2021 20:27
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.

2 participants