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!: remove supportsInterface #352

Merged
merged 1 commit into from
Nov 23, 2023
Merged

refactor!: remove supportsInterface #352

merged 1 commit into from
Nov 23, 2023

Conversation

fhildeb
Copy link
Collaborator

@fhildeb fhildeb commented Nov 9, 2023

What kind of change does this PR introduce?

Braking Feature Change: This PR removes the this.supportsInterface(...) and ERC725.supportsInterface(...) functions, as well as their tests and documentation.

What is the current behavior?

Currently, erc725.js can be used to resolve and detect whitelisted ERC165-based interface IDs of a given contract based on their standardized string inputs:

// Object
myErc725.supportsInterface("ERC725X")
myErc725.supportsInterface("LSP8IdentifiableDigitalAsset")

// Class
ERC725.supportsInterface("ERC725X", "0x...")
ERC725.supportsInterface("LSP8IdentifiableDigitalAsset", "0x...")

While this is a convenient way to check interfaces without further redo, resolving and manually updating IDs can be cumbersome.

What is the new behavior?

The decision was made to completely remove this functionality and keep it completely separated from contract libraries (like lsp-smart-contracts), that feature additional information like ABIs and schemas on top of being up-to-date with the latest standardization changes.

If you are affected by this breaking feature change, please use the following method to read ERC165-based interface IDs:

// Example using IDs from the @lukso/lsp-smart-contracts library
import { INTERFACE_IDS } from "@lukso/lsp-smart-contracts";

// Define a global EIP165 ABI 
const eip165ABI = [{
    inputs: [{
        internalType: 'bytes4',
        name: 'interfaceId',
        type: 'bytes4',
      }],
    name: 'supportsInterface',
    outputs: [{
        internalType: 'bool',
        name: '',
        type: 'bool',
      }],
    stateMutability: 'view',
    type: 'function',
  }];
  
  try {
    const myEip165Contract = contract(eip165ABI as any, address, provider)
    return await eip165Contract.methods.supportsInterface(INTERFACE_IDS.ERC725X).call()
    return await eip165Contract.methods.supportsInterface(INTERFACE_IDS.LSP8IdentifiableDigitalAsset).call()
  } catch (error: unknown) {
    console.error(error)
    return false
  }

Other information:

The following repositories have been checked:
erc725-inspect
my.universalprofile.cloud
wallet.universalprofile.cloud

None of them is using ERC725's supportsInterface function anymore.

@codecov-commenter
Copy link

Codecov Report

Merging #352 (ab0db08) into develop (9640d9f) will decrease coverage by 0.21%.
Report is 15 commits behind head on develop.
The diff coverage is 73.52%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop     #352      +/-   ##
===========================================
- Coverage    83.71%   83.51%   -0.21%     
===========================================
  Files           18       17       -1     
  Lines         1130     1122       -8     
  Branches       255      258       +3     
===========================================
- Hits           946      937       -9     
+ Misses          98       94       -4     
- Partials        86       91       +5     
Files Coverage Δ
src/constants/constants.ts 100.00% <100.00%> (ø)
src/index.ts 78.12% <100.00%> (+3.12%) ⬆️
src/lib/detector.ts 100.00% <ø> (+4.00%) ⬆️
src/lib/getDataFromExternalSources.ts 61.29% <33.33%> (-2.05%) ⬇️
src/lib/utils.ts 82.09% <81.81%> (-1.03%) ⬇️
src/lib/encoder.ts 82.78% <64.28%> (-1.18%) ⬇️

@CJ42
Copy link
Collaborator

CJ42 commented Nov 9, 2023

This could be duplicate with #317
I had opened the PR already, but we could close it and keep this one (or the other way around)

@fhildeb fhildeb changed the title chore: remove supportsInterface refactor: remove supportsInterface Nov 9, 2023
@fhildeb
Copy link
Collaborator Author

fhildeb commented Nov 9, 2023

Oh you are right. Did not check if there already was a PR open for that. 😅 Has there been an outcome yet? @frozeman

@CJ42
Copy link
Collaborator

CJ42 commented Nov 9, 2023

From my side, I would go ahead with this PR so that we focus this library purely on the encoding / decoding side.
Which is what this library is aimed for.

@Hugoo
Copy link
Contributor

Hugoo commented Nov 10, 2023

Nice that the other repos were updated.

If this is a breaking change, don't forget to add ! in the commit name.

refactor!: remove supportsInterface

https://www.conventionalcommits.org/en/v1.0.0/

@Hugoo Hugoo changed the title refactor: remove supportsInterface refactor!: remove supportsInterface Nov 10, 2023
@CJ42 CJ42 mentioned this pull request Nov 20, 2023
@Hugoo Hugoo changed the base branch from develop to remove-interface-checks November 23, 2023 14:57
@Hugoo Hugoo changed the base branch from remove-interface-checks to develop November 23, 2023 15:01
@Hugoo Hugoo merged commit ab83f08 into ERC725Alliance:develop Nov 23, 2023
2 checks passed
@Hugoo Hugoo deleted the remove-interface-checks branch November 23, 2023 15:02
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