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 280: create context-module package for clear signing #6788

Merged
merged 1 commit into from May 16, 2024
Merged

Feat/dsdk 280: create context-module package for clear signing #6788

merged 1 commit into from May 16, 2024

Conversation

aussedatlo
Copy link
Contributor

@aussedatlo aussedatlo commented Apr 30, 2024 •
edited by jira bot

✅ Checklist

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

📝 Description

Create the context-module package that will be used by the future keyring-eth to get all the context of a transaction using different backend.
This ticket is for:

❓ 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 11:10am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 11:10am
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 11:10am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 11:10am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 16, 2024 11:10am

@live-github-bot live-github-bot bot added the ledgerjs Has changes in the ledgerjs open source libs label Apr 30, 2024
@aussedatlo aussedatlo changed the title feat: create context-module package for clear signing feat/DSDK 280: create context-module package for clear signing Apr 30, 2024
@aussedatlo aussedatlo changed the title feat/DSDK 280: create context-module package for clear signing Feat/dsdk 280: create context-module package for clear signing Apr 30, 2024
lambertkevin
lambertkevin previously approved these 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.

Just 2 minor things, other than that, all good 👍

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
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from ccf918a to b9d0fde Compare May 2, 2024 11:26
mbertin-ledger
mbertin-ledger previously approved these changes May 3, 2024
View reviewed changes
Copy link
Member

@mbertin-ledger mbertin-ledger left a comment

Choose a reason for hiding this comment

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

👍
Good Overral.
My only comment is about the loaders naming, for me it's not very easy to understand the purpose, I would prefer using resolvers.

libs/ledgerjs/packages/context-module/src/ContextModule.ts Show resolved Hide resolved
libs/ledgerjs/packages/context-module/src/DefaultContextModule.ts Show resolved Hide resolved
libs/ledgerjs/packages/context-module/src/models/ContextResponse.ts Outdated Show resolved Hide resolved
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from b9d0fde to 23d6f23 Compare May 3, 2024 09:19
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from 23d6f23 to fab39b4 Compare May 3, 2024 12:36
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from fab39b4 to 7f7f196 Compare May 7, 2024 07:24
@socket-security Socket Security
Copy link

socket-security bot commented May 7, 2024 •
edited

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@tsconfig/recommended@1.0.6 None 0 2.32 kB typescript-deploys

View full report↗︎

@aussedatlo
Copy link
Contributor Author

Rebased with a new architecture, by scope

mbertin-ledger
mbertin-ledger previously approved these changes May 14, 2024
View reviewed changes
@live-github-bot
Copy link
Contributor

live-github-bot bot commented May 15, 2024 •
edited

Mobile Bundle Checks

Comparing 870818e against 8a065af.

✅ Previous issues have all been fixed.

Desktop Bundle Checks

Comparing 870818e against 2e989ad.

✅ Previous issues have all been fixed.

lambertkevin
lambertkevin previously approved these changes May 15, 2024
View reviewed changes
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from 7f7f196 to 56b0861 Compare May 16, 2024 09:51
@aussedatlo aussedatlo force-pushed the feat/dsdk-281-context-module branch from 56b0861 to 870818e Compare May 16, 2024 10:21
@aussedatlo aussedatlo merged commit 0d0564e into develop May 16, 2024
56 checks passed
@aussedatlo aussedatlo deleted the feat/dsdk-281-context-module branch May 16, 2024 11:28
Vin757
Vin757 reviewed May 17, 2024
View reviewed changes
Copy link

@Vin757 Vin757 left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vin757 Vin757 Vin757 left review comments

@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
4 participants
@aussedatlo @lambertkevin @mbertin-ledger @Vin757