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

refactor!: deprecate static / non-static supportsInterface methods #317

Closed
wants to merge 1 commit into from

Conversation

CJ42
Copy link
Collaborator

@CJ42 CJ42 commented Oct 17, 2023

What kind of change does this PR introduce (bug fix, feature, docs update, ...)?

Deprecation

What is the current behaviour (you can also link to an open issue here)?

The erc725.js library contains a list of interface IDs related to the @lukso/lsp-smart-contracts. These interface IDs are out of date and never maintain, and should not be in this package, but instead imported as:

import { INTERFACE_IDS } from "@lukso/lsp-smart-contracts";

And simply checked using a ERC165 ABI

  try {
    const eip165Contract = contract(eip165ABI as any, address)
    return await eip165Contract.methods.supportsInterface(interfaceId).call()
  } catch (error: unknown) {
    console.error(error)
    return false
  }

What is the new behaviour (if this is a feature change)?

Deprecate the function supportsInterface from the erc725.js library, to avoid future errors and maintenance for a function that is not used and should not be used.

Other information:

Copy link
Contributor

@frozeman frozeman left a comment

Choose a reason for hiding this comment

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

Lets please discuss, as this is valid feature but should not work like solidly but rather:

erc725js.supporstInterface('ERC725X')
erc725js.supporstInterface('LSP0')
...

dapp developers need to be able to check with contract they are dealing with

@CallumGrindle
Copy link
Collaborator

@CJ42 @frozeman I agree this is a useful function for developers, but I also think erc725.js is the wrong place for it.

We can put this in the lsp-utils lib or something more appropriate, otherwise we have a weird dependency between this lib and the the lsp versions which we should aim to remove. Ideally erc725.js should only need to be updated when there is change to ERC725 or an LSP2 schema. Currently we need to update it every time there is an lsp-smart-contracts release, which is a design smell imo.

This also applies to other functions like encodePermissions, which can also live in lsp-utils.

@Hugoo
Copy link
Contributor

Hugoo commented Nov 10, 2023

It currently works with both - interfaceId (hex) and name.

image

I agree this might belong to another package as this is not really erc725 related.

We have no rush to remove that and I think we use this feature in other apps. Maybe let's wait until it is avail. in an other lib - such as lsp-utils before removing it from here?

@CJ42 CJ42 mentioned this pull request Nov 20, 2023
@Hugoo
Copy link
Contributor

Hugoo commented Nov 23, 2023

I close because it is a duplicate of: #352

@Hugoo Hugoo closed this Nov 23, 2023
@Hugoo Hugoo deleted the refactor/deprecate-supportsinterface branch November 23, 2023 14:55
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.

4 participants