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

chore: use exotics api v2 #298

Merged
merged 5 commits into from
Nov 29, 2023
Merged

chore: use exotics api v2 #298

merged 5 commits into from
Nov 29, 2023

Conversation

fedeerbes
Copy link
Contributor

@fedeerbes fedeerbes commented Nov 27, 2023

πŸ”˜ PR Type

  • Bugfix
  • Enhancement
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

πŸ“œ Background

This PR implements the v2 of exotics API and adds an util to map API response to Bundle

πŸ”„ Changes

Does this PR introduce a breaking change?

  • Yes, Incompatible API changes
  • No, Adds functionality (backwards compatible)
  • No, Bug fixes (backwards compatible)

Changes:

  • add v2 of exotics API with types (breaking change)
  • add mapRareSatsAPIResponseToBundle and test (non breaking change)

βœ… Review checklist

Please ensure the following are true before merging:

  • Code Style is consistent with the project guidelines.
  • Code is readable and well-commented.
  • No unnecessary or debugging code has been added.
  • Security considerations have been taken into account.
  • The change has been manually tested and works as expected.
  • Breaking changes and their impacts have been considered and documented.
  • Code does not introduce new technical debt or issues.

@fedeerbes fedeerbes self-assigned this Nov 27, 2023
@fedeerbes fedeerbes marked this pull request as ready for review November 27, 2023 15:51
Copy link
Member

@teebszet teebszet left a comment

Choose a reason for hiding this comment

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

Lgtm

Comment on lines 41 to 42
export const RoadArmorRareSats = ['MYTHIC', 'LEGENDARY', 'EPIC', 'RARE', 'UNCOMMON', 'COMMON'] as const;
export type RoadArmorRareSatsType = (typeof RoadArmorRareSats)[number];
Copy link
Member

Choose a reason for hiding this comment

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

rarity_ranking: SatRarity;
offset: number;
};
export const Sattributes = [
Copy link
Member

Choose a reason for hiding this comment

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

nit: we use "satribute" in api code

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I initially wrote it sattributes everywhere as well because English πŸ˜‹ All the market places use a single 't' though, so we went with that for consistency.

api/ordinals.ts Outdated Show resolved Hide resolved
Comment on lines 1 to 2
import { mapRareSatsAPIResponseToBundle } from 'api';
import { Bundle, UtxoOrdinalBundle } from 'types';
Copy link
Member

Choose a reason for hiding this comment

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

Please make these relative imports

api/ordinals.ts Outdated
totalSats: apiBundle.value,
};

// if bundle has and empty sat ranges, it means that it's a common/unknown bundle
Copy link
Member

Choose a reason for hiding this comment

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

nit πŸ‘€

Suggested change
// if bundle has and empty sat ranges, it means that it's a common/unknown bundle
// if bundle has empty sat ranges, it means that it's a common/unknown bundle

api/ordinals.ts Outdated
);

const rangeWithUnsupportedSatsAndWithoutInscriptions =
satRange.satributes.length === 1 && !satRange.inscriptions.length && !supportedSatributes.length;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
satRange.satributes.length === 1 && !satRange.inscriptions.length && !supportedSatributes.length;
!satRange.inscriptions.length && !supportedSatributes.length;

api/ordinals.ts Outdated
yearMined,
satributes,
// only one inscription per range is supported
inscriptions: satRange.inscriptions.length > 1 ? [satRange.inscriptions[0]] : satRange.inscriptions,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we only support one? πŸ‘€

api/ordinals.ts Outdated

// if totalExoticSats doesn't match the value of the bundle,
// it means that the bundle is not fully exotic and we need to add a common unknown sat range
if (totalExoticSats !== apiBundle.value) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (totalExoticSats !== apiBundle.value) {
if (totalExoticSats + totalCommonUnknownInscribedSats !== apiBundle.value) {

export type Bundle = Omit<UtxoOrdinalBundle, 'sat_ranges'> & {
satRanges: BundleSatRange[];
inscriptions: SatRangeInscription[];
satributes: RareSatsType[][];
Copy link
Member

Choose a reason for hiding this comment

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

This is the return type of the API to bundle mapping. At this point, any unknown satributes would have been filtered out.

Maybe we should have RareSatsType as just RodarmorRareSatsType | SatributesType and use that here. Then we can have something like FuzzyRareSatTypes (please choose a better name πŸ˜… ) which is RareSatsType | string which we can use for the API response type.

Copy link
Contributor

Test this PR with npm i --save-exact @secretkeylabs/xverse-core@3.1.1-dc11cba

Copy link
Member

@victorkirov victorkirov left a comment

Choose a reason for hiding this comment

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

πŸš€ Nice!

@fedeerbes fedeerbes merged commit cdc27ff into develop Nov 29, 2023
3 checks passed
@fedeerbes fedeerbes deleted the chore/exotics-api-v2 branch November 29, 2023 14:10
@github-actions github-actions bot mentioned this pull request Nov 29, 2023
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.

None yet

3 participants