Skip to content

Disallow unsafe transactions#757

Merged
Agusx1211 merged 1 commit into
masterfrom
test-wdk
May 21, 2025
Merged

Disallow unsafe transactions#757
Agusx1211 merged 1 commit into
masterfrom
test-wdk

Conversation

@Agusx1211

Copy link
Copy Markdown
Member

No description provided.

@Agusx1211 Agusx1211 requested a review from Copilot May 21, 2025 08:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Adds an unsafe flag to transaction workflows to allow bypassing safe-mode restrictions (delegate calls and self-calls).

  • Extends the transaction API (Manager and Transactions) with an unsafe option.
  • Implements safe-mode checks in Wallet.prepareTransaction, throwing if delegate calls or self-calls occur when unsafe is unset.
  • Adds tests to verify that self-calls are rejected in safe mode and allowed in unsafe mode.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
packages/wallet/wdk/test/transactions.test.ts New tests for rejecting self-calls in safe mode and allowing in unsafe mode
packages/wallet/wdk/src/sequence/transactions.ts Added unsafe option to the requestTransaction method signature
packages/wallet/wdk/src/sequence/manager.ts Updated Manager.request signature to include unsafe
packages/wallet/core/src/wallet.ts Added safe-mode validation logic in prepareTransaction and unsafe option
Comments suppressed due to low confidence (4)

packages/wallet/wdk/test/transactions.test.ts:465

  • [nitpick] The test description mentions 'unsafe transactions' but specifically covers self-calls. Consider renaming to 'Should reject self-call transactions in safe mode' for clarity.
it('Should reject unsafe transactions in safe mode (call to self)', async () => {

packages/wallet/wdk/test/transactions.test.ts:482

  • Consider adding a parallel test to verify that delegateCall: true is rejected in safe mode, ensuring coverage of both restriction rules.
it('Should allow transactions to self in unsafe mode', async () => {

packages/wallet/core/src/wallet.ts:256

  • [nitpick] Capitalize the message and optionally guide override: 'Delegate calls are not allowed in safe mode. To override, set unsafe: true.'
throw new Error('delegate calls are not allowed in safe mode')

packages/wallet/core/src/wallet.ts:259

  • [nitpick] Capitalize the message and suggest override: 'Calls to the wallet contract itself are not allowed in safe mode. To override, set unsafe: true.'
throw new Error('calls to the wallet contract itself are not allowed in safe mode')

Comment on lines +249 to +251
// If safe mode is set, then we check that the transaction
// is not "dangerous", aka it does not have any delegate calls
// or calls to the wallet contract itself

Copilot AI May 21, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Consider replacing the informal 'aka' with 'i.e.' and clarifying: 'When unsafe is unset, disallow delegate calls or self-calls.'

Suggested change
// If safe mode is set, then we check that the transaction
// is not "dangerous", aka it does not have any delegate calls
// or calls to the wallet contract itself
// If safe mode is set (i.e., when `unsafe` is unset), disallow
// delegate calls and calls to the wallet contract itself.

Copilot uses AI. Check for mistakes.
@Agusx1211 Agusx1211 marked this pull request as ready for review May 21, 2025 08:37
@Agusx1211 Agusx1211 requested review from a team as code owners May 21, 2025 08:37
@Agusx1211 Agusx1211 merged commit 03e9912 into master May 21, 2025
3 checks passed
@Agusx1211 Agusx1211 deleted the test-wdk branch May 21, 2025 08:37
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.

2 participants