Skip to content

Remove WalletView infavor of individual methods#25

Merged
arminsabouri merged 1 commit intomasterfrom
rm-wallet-view
Apr 21, 2026
Merged

Remove WalletView infavor of individual methods#25
arminsabouri merged 1 commit intomasterfrom
rm-wallet-view

Conversation

@arminsabouri
Copy link
Copy Markdown
Collaborator

Since we use wallet view in our strategies and actions (as of #12 ) there is no strong reason for wallet view to also exist. In a sense its duplicate data.
closes #24
Most of the refactoring was done by claude sonnet

@arminsabouri arminsabouri force-pushed the rm-wallet-view branch 2 times, most recently from e5d3fcb to d0cdf9d Compare April 17, 2026 20:58
Copy link
Copy Markdown
Contributor

@0xZaddyy 0xZaddyy left a comment

Choose a reason for hiding this comment

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

everything looks clean and consistent

Ack d0cdf9d

Copy link
Copy Markdown
Contributor

@Mshehu5 Mshehu5 left a comment

Choose a reason for hiding this comment

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

cACK but currently strategies take &WalletHandleMut even though they appear to only read state. Given the existing WalletHandle/WalletHandleMut split should we move strategies to &WalletHandle in a follow-up PR so we have clear read/write boundary? Or if WalletHandle is no longer intended to be used it should probably be removed in a follow-up instead.

@arminsabouri
Copy link
Copy Markdown
Collaborator Author

cACK but currently strategies take &WalletHandleMut even though they appear to only read state. Given the existing WalletHandle/WalletHandleMut split should we move strategies to &WalletHandle in a follow-up PR so we have clear read/write boundary? Or if WalletHandle is no longer intended to be used it should probably be removed in a follow-up instead.

Good call! This has been addressed in e2d0f96

Copy link
Copy Markdown
Contributor

@Mshehu5 Mshehu5 left a comment

Choose a reason for hiding this comment

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

Thank you for working on this !
ACK e2d0f96

@arminsabouri arminsabouri merged commit 3b88203 into master Apr 21, 2026
1 check passed
@arminsabouri arminsabouri deleted the rm-wallet-view branch April 21, 2026 13:19
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.

actions use both walletview and mut wallet handle

3 participants