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

Multichain and modeling refactor #22

Merged
merged 17 commits into from
Aug 9, 2022
Merged

Conversation

vickygos
Copy link
Contributor

@vickygos vickygos commented Aug 3, 2022

Changes

  • Refactors and updates library to use proposed interface approved in the following RFC: https://www.notion.so/opensea/Extending-OpenRarity-to-non-EVM-chains-fad5ff855f9c4492aa24d4f3f02b51e6
    (UML Diagram)
    • Updates core models Token and Collection to use the proposed interface that is multichain and interface friendly
    • Creates new interfaces, enums and models: TokenIdentifier
    • Update all tests and resolver logic that assumes the original token/collection model
  • Refactors out the resolver logic (which is used to compare rarity ranks across different providers) from the core library interface
    • Previously the resolver logic was oddly modifying the input, whereas in the new system, the input is not modified/touched by the core library -> it just outputs the rank
    • Created new models CollectionWithMetadata and TokenWithRarityData to specifically help with rarity resolvers and comparing rarity
    • Cleanup/Refactor external_rarity_provider, cleaning splitting out independent helper methods with modifiers for comparisons
  • Updated the various openrarity scoring algorithms to be sub-classes of Scorer, and updated interface to properly score tokens based on collection + token inputs (and not modify said inputs)
  • Did a general cleanup/refactor where it applied, without trying to yak-shave too much....

Open questions:

  • What final scoring interface do we want? With current model, the library user would get to decide which scoring strategy to use. However, we may want to provide a wrapper like "OpenRarityScorer" that just uses our chosen scorer option. cc @impreso

For follow-up PRs:

  • @impreso to update all unit tests - a lot of the existing ones are incorrect logic
  • @vickygos to follow-up on some TODO's mainly removing the count in StringAttributeValue

.gitignore Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
open_rarity/resolver/opensea_api_helpers.py Show resolved Hide resolved
open_rarity/resolver/opensea_api_helpers.py Show resolved Hide resolved
open_rarity/resolver/opensea_api_helpers.py Show resolved Hide resolved
open_rarity/resolver/opensea_api_helpers.py Show resolved Hide resolved
open_rarity/scoring/utils.py Show resolved Hide resolved
tests/test_scoring.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@impreso impreso left a comment

Choose a reason for hiding this comment

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

LGTM left additional feedback, but overall makes sense.

@vickygos vickygos merged commit 71bad47 into main Aug 9, 2022
@vickygos vickygos deleted the multichain_and_modeling_refactor branch August 9, 2022 22:45
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.

3 participants