-
Notifications
You must be signed in to change notification settings - Fork 158
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: upgrade viem to support heterogeneous calls #1527
base: main
Are you sure you want to change the base?
Changes from 9 commits
f68168b
f4fe453
7580239
70a0ebb
2ab58c8
574e62b
337fd00
3128ec4
b246423
8731574
323b49e
8b2bb4a
76a5437
57bce79
2099755
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,6 @@ export function Transaction({ | |
chainId, | ||
className, | ||
children, | ||
contracts, | ||
isSponsored, | ||
onError, | ||
onStatus, | ||
|
@@ -35,7 +34,6 @@ export function Transaction({ | |
calls={calls} | ||
capabilities={capabilities} | ||
chainId={accountChainId} | ||
contracts={contracts} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we still support this param and/or deprecate over time by just having anything passed in here be treated the same as the calls param? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm leaning towards just removing it to avoid confusion. i.e devs might think they need to pass calls to calls and contracts to contracts which would cause the order of execution to be unreliable. plus it is a super small change on the developer side (just changing prop name to calls). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you left the calls and contracts error in place we could mark contracts as deprecated for a few releases before removing. it would at least give the devs a heads up over the next few releases before the forcing them to update due to a breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious what the rest of the team thinks but personally still leaning towards just removing it since it is such a small change on the dev side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on marking as deprecated for a few releases, then removing. It should be a simple change for the dev but nice to give them a bit of time to switch over There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, reconsidered this. Given it's a simple prop name change, I'd probably lean towards just making the breaking change now |
||
isSponsored={isSponsored} | ||
onError={onError} | ||
onStatus={onStatus} | ||
|
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.
are we still using this dependency after these changes (and under dependencies)?