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.

Sign up for GitHub

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

Jump to bottom

Feat/dsdk 285 create context module nft loaders #6789

Merged
merged 2 commits into from May 16, 2024
Merged

Feat/dsdk 285 create context module nft loaders #6789

merged 2 commits into from May 16, 2024

Conversation

aussedatlo
Copy link
Contributor

@aussedatlo aussedatlo commented Apr 30, 2024 •
edited

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

📝 Description

Implement DomainLoader and NftLoader to get metadatas used to cleasign a transaction.
This PR correspond to:

❓ Context

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@vercel Vercel
Copy link

vercel bot commented Apr 30, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 10:29am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 10:29am
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 10:29am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 10:29am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 10:29am

@live-github-bot live-github-bot bot added the ledgerjs Has changes in the ledgerjs open source libs label Apr 30, 2024
@aussedatlo aussedatlo force-pushed the feat/dsdk-285-context-module-nft branch from 8261cc8 to a0a7bc9 Compare April 30, 2024 15:29
@aussedatlo aussedatlo changed the title Feat/dsdk 285 context module nft Feat/dsdk 285 create context module nft loaders Apr 30, 2024
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from ccf918a to b9d0fde Compare May 2, 2024 11:26
lambertkevin
lambertkevin requested changes May 2, 2024
View reviewed changes
Copy link
Contributor

@lambertkevin lambertkevin left a comment

Choose a reason for hiding this comment

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

Only 1 change that seems required because I think we're always throwing instead of just letting it go through for domains. Other than that, it's just minor recommendations 👍

libs/ledgerjs/packages/context-module/package.json Outdated Show resolved Hide resolved
libs/ledgerjs/packages/context-module/src/DefaultContextModule.ts Outdated Show resolved Hide resolved
libs/ledgerjs/packages/context-module/src/loaders/ForwardDomainLoader.ts Outdated Show resolved Hide resolved
libs/ledgerjs/packages/context-module/src/loaders/ForwardDomainLoader.ts Outdated Show resolved Hide resolved
libs/ledgerjs/packages/context-module/src/loaders/NftLoader.test.ts Outdated Show resolved Hide resolved
@aussedatlo aussedatlo force-pushed the feat/dsdk-285-context-module-nft branch from a0a7bc9 to 5fe1e51 Compare May 3, 2024 08:28
@aussedatlo aussedatlo force-pushed the feat/dsdk-285-context-module-nft branch from 5fe1e51 to 0938823 Compare May 3, 2024 08:52
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch 3 times, most recently from fab39b4 to 7f7f196 Compare May 7, 2024 07:24
@aussedatlo aussedatlo force-pushed the feat/dsdk-285-context-module-nft branch from 0938823 to ecaf487 Compare May 7, 2024 07:34
mbertin-ledger
mbertin-ledger previously approved these changes May 14, 2024
View reviewed changes
lambertkevin
lambertkevin previously approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@lambertkevin lambertkevin left a comment

Choose a reason for hiding this comment

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

Just 1 minor & optional addition. Otherwise 👍

...edgerjs/packages/context-module/src/forward-domain/domain/ForwardDomainContextLoader.test.ts Outdated Show resolved Hide resolved
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch 2 times, most recently from 56b0861 to 870818e Compare May 16, 2024 10:21
@aussedatlo aussedatlo force-pushed the feat/dsdk-285-context-module-nft branch from ecaf487 to 550ff5f Compare May 16, 2024 10:25
Base automatically changed from feat/dsdk-281-context-module to develop May 16, 2024 11:28
@aussedatlo aussedatlo dismissed stale reviews from lambertkevin and mbertin-ledger May 16, 2024 11:28

The base branch was changed.

@aussedatlo aussedatlo merged commit e541abf into develop May 16, 2024
32 checks passed
@aussedatlo aussedatlo deleted the feat/dsdk-285-context-module-nft branch May 16, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lambertkevin lambertkevin lambertkevin approved these changes

@mbertin-ledger mbertin-ledger mbertin-ledger left review comments

Assignees

@aussedatlo aussedatlo

Labels
ledgerjs Has changes in the ledgerjs open source libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aussedatlo @lambertkevin @mbertin-ledger