-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat(x/warden): remove SignTransactionRequests #173
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughWalkthroughThe changes represent a strategic pivot towards address management over transaction signing. This transition involves eliminating transaction signing capabilities, introducing new address type enums, and adapting APIs and components to prioritize address-related operations. The incorporation of new dependencies and reorganization throughout components and hooks reflects this fundamental shift in focus. Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
We can reuse SignatureRequests for everything, and add a "SignMethod" input field when creating it to instruct how to parse the input []byte. This has several benefits: - less code - less entities in the appdb, especially large ones containing full ethereum or L1 transactions - cleaner API surface
Cleanup dead code, rename "wallets" into "addresses" for clarity and decouple it from transaction parsing (now called SignMethod).
c79b4eb
to
a67e818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-arm-gnueabihf@4.9.6: | ||
resolution: {integrity: sha512-oNk8YXDDnNyG4qlNb6is1ojTOGL/tRhbbKeE/YuccItzerEZT68Z9gHrY3ROh7axDc974+zYAPxK5SH0j/G+QQ==} | ||
/@rollup/rollup-linux-arm-gnueabihf@4.14.3: | ||
resolution: {integrity: sha512-ge2DC7tHRHa3caVEoSbPRJpq7azhG+xYsd6u2MEnJ6XzPSzQsTKyXvh6iWjXRf7Rt9ykIUWHtl0Uz3T6yXPpKw==} | ||
cpu: [arm] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-arm-musleabihf@4.14.3: | ||
resolution: {integrity: sha512-ljcuiDI4V3ySuc7eSk4lQ9wU8J8r8KrOUvB2U+TtK0TiW6OFDmJ+DdIjjwZHIw9CNxzbmXY39wwpzYuFDwNXuw==} | ||
cpu: [arm] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-arm64-gnu@4.9.6: | ||
resolution: {integrity: sha512-Z3O60yxPtuCYobrtzjo0wlmvDdx2qZfeAWTyfOjEDqd08kthDKexLpV97KfAeUXPosENKd8uyJMRDfFMxcYkDQ==} | ||
/@rollup/rollup-linux-arm64-gnu@4.14.3: | ||
resolution: {integrity: sha512-Eci2us9VTHm1eSyn5/eEpaC7eP/mp5n46gTRB3Aar3BgSvDQGJZuicyq6TsH4HngNBgVqC5sDYxOzTExSU+NjA==} | ||
cpu: [arm64] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-arm64-musl@4.9.6: | ||
resolution: {integrity: sha512-gpiG0qQJNdYEVad+1iAsGAbgAnZ8j07FapmnIAQgODKcOTjLEWM9sRb+MbQyVsYCnA0Im6M6QIq6ax7liws6eQ==} | ||
/@rollup/rollup-linux-arm64-musl@4.14.3: | ||
resolution: {integrity: sha512-UrBoMLCq4E92/LCqlh+blpqMz5h1tJttPIniwUgOFJyjWI1qrtrDhhpHPuFxULlUmjFHfloWdixtDhSxJt5iKw==} | ||
cpu: [arm64] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-riscv64-gnu@4.9.6: | ||
resolution: {integrity: sha512-+uCOcvVmFUYvVDr27aiyun9WgZk0tXe7ThuzoUTAukZJOwS5MrGbmSlNOhx1j80GdpqbOty05XqSl5w4dQvcOA==} | ||
/@rollup/rollup-linux-powerpc64le-gnu@4.14.3: | ||
resolution: {integrity: sha512-5aRjvsS8q1nWN8AoRfrq5+9IflC3P1leMoy4r2WjXyFqf3qcqsxRCfxtZIV58tCxd+Yv7WELPcO9mY9aeQyAmw==} | ||
cpu: [ppc64] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-riscv64-gnu@4.14.3: | ||
resolution: {integrity: sha512-sk/Qh1j2/RJSX7FhEpJn8n0ndxy/uf0kI/9Zc4b1ELhqULVdTfN6HL31CDaTChiBAOgLcsJ1sgVZjWv8XNEsAQ==} | ||
cpu: [riscv64] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-x64-gnu@4.9.6: | ||
resolution: {integrity: sha512-HUNqM32dGzfBKuaDUBqFB7tP6VMN74eLZ33Q9Y1TBqRDn+qDonkAUyKWwF9BR9unV7QUzffLnz9GrnKvMqC/fw==} | ||
/@rollup/rollup-linux-s390x-gnu@4.14.3: | ||
resolution: {integrity: sha512-jOO/PEaDitOmY9TgkxF/TQIjXySQe5KVYB57H/8LRP/ux0ZoO8cSHCX17asMSv3ruwslXW/TLBcxyaUzGRHcqg==} | ||
cpu: [s390x] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-x64-gnu@4.14.3: | ||
resolution: {integrity: sha512-8ybV4Xjy59xLMyWo3GCfEGqtKV5M5gCSrZlxkPGvEPCGDLNla7v48S662HSGwRd6/2cSneMQWiv+QzcttLrrOA==} | ||
cpu: [x64] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-linux-x64-musl@4.9.6: | ||
resolution: {integrity: sha512-ch7M+9Tr5R4FK40FHQk8VnML0Szi2KRujUgHXd/HjuH9ifH72GUmw6lStZBo3c3GB82vHa0ZoUfjfcM7JiiMrQ==} | ||
/@rollup/rollup-linux-x64-musl@4.14.3: | ||
resolution: {integrity: sha512-s+xf1I46trOY10OqAtZ5Rm6lzHre/UiLA1J2uOhCFXWkbZrJRkYBPO6FhvGfHmdtQ3Bx793MNa7LvoWFAm93bg==} | ||
cpu: [x64] | ||
os: [linux] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-win32-arm64-msvc@4.9.6: | ||
resolution: {integrity: sha512-VD6qnR99dhmTQ1mJhIzXsRcTBvTjbfbGGwKAHcu+52cVl15AC/kplkhxzW/uT0Xl62Y/meBKDZvoJSJN+vTeGA==} | ||
/@rollup/rollup-win32-arm64-msvc@4.14.3: | ||
resolution: {integrity: sha512-+4h2WrGOYsOumDQ5S2sYNyhVfrue+9tc9XcLWLh+Kw3UOxAvrfOrSMFon60KspcDdytkNDh7K2Vs6eMaYImAZg==} | ||
cpu: [arm64] | ||
os: [win32] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-win32-ia32-msvc@4.9.6: | ||
resolution: {integrity: sha512-J9AFDq/xiRI58eR2NIDfyVmTYGyIZmRcvcAoJ48oDld/NTR8wyiPUu2X/v1navJ+N/FGg68LEbX3Ejd6l8B7MQ==} | ||
/@rollup/rollup-win32-ia32-msvc@4.14.3: | ||
resolution: {integrity: sha512-T1l7y/bCeL/kUwh9OD4PQT4aM7Bq43vX05htPJJ46RTI4r5KNt6qJRzAfNfM+OYMNEVBWQzR2Gyk+FXLZfogGw==} | ||
cpu: [ia32] | ||
os: [win32] | ||
requiresBuild: true | ||
optional: true | ||
|
||
/@rollup/rollup-win32-x64-msvc@4.9.6: | ||
resolution: {integrity: sha512-jqzNLhNDvIZOrt69Ce4UjGRpXJBzhUBzawMwnaDAwyHriki3XollsewxWzOzz+4yOFDkuJHtTsZFwMxhYJWmLQ==} | ||
/@rollup/rollup-win32-x64-msvc@4.14.3: | ||
resolution: {integrity: sha512-/BypzV0H1y1HzgYpxqRaXGBRqfodgoBBCcsrujT6QRcakDQdfU+Lq9PENPh5jB4I44YWq+0C2eHsHya+nZY1sA==} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The output from the executed script confirms that the @rollup
packages listed in the pnpm-lock.yaml
file are indeed set to version 4.14.3
across various platforms and architectures. Additionally, the script output indicates that the @rollup/rollup-win32-x64-msvc
package is deprecated and no longer maintained, with a recommendation to use @rollup/plugin-inject
instead.
Given this information, it appears that the versions and configurations for the @rollup
packages are correctly set as per the snippet provided. However, the deprecation of the @rollup/rollup-win32-x64-msvc
package and the recommendation to switch to another package should be addressed to ensure future compatibility and maintenance.
Conclusion
The versions and configurations for the @rollup
packages are correctly set as per the provided snippet. However, attention should be given to the deprecation notice for the @rollup/rollup-win32-x64-msvc
package. It is recommended to consider transitioning to the suggested @rollup/plugin-inject
package for future updates and maintenance.
Analysis chain
Ensure the new versions and configurations for @rollup
packages are correctly set and compatible with the project.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Ensure the new versions and configurations for `@rollup` packages are correctly set.
rg '@rollup' spaceward/pnpm-lock.yaml
Length of output: 1746
SpaceWard builds locally but not in Docker or in GitHub Action. I'll continue my investigation to fix that before merging this PR. |
I removed SignTransactionRequests and kept SignatureRequests only.
SignTransactionRequests consisted in a step that parsed the input binary as a L1 transaction and generated a corresponding SignatureRequest.
I simplified this by adding a "SignMethod" field to NewSignatureRequest, so that the user can specify directly how the input should be treated. A SignMethod called "black box" can be used when the input should be used as-is.
This has several benefits, for example we don't store anymore the entire L1 unsigned transactions in the app.db, saving some space. And it also reduces and clarifies our exposed APIs.
I fixed SpaceWard to use the new methods and types, and I started using our new
wardjs
client instead ofts-client
. While doing that I found some little TypeScript/codegen problems with Telescope that I fixed in this PR.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Dependencies