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.

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

refactor: use getAction internally #2627

Closed
wants to merge 3 commits into from

Conversation

holic
Copy link
Contributor

@holic holic commented Aug 17, 2024

As an author of custom actions and action overrides, I will use viem actions internally but sometimes forget to call them getAction so that the actions can compose on one another.

For example, maybe I'll use getChainId internally in my custom action, but I want this to be composable with an action that overrides getChainId to cache the RPC's chain ID. If I don't call this with getAction, I'll accidentally bypass this cache and viem will do the underlying RPC call.

I've seen this pattern in both in my code, other SDKs, and within viem itself.

I am curious if, instead of requiring all action authors to use getAction, that viem itself exports by default the getAction-wrapped variant of each of these actions, while also exporting the internal implementation in case it's useful to make an explicit, direct call.

I'm mocking this up here to see if this approach seems reasonable before doing more because its quite a big refactor.


PR-Codex overview

This PR focuses on refactoring functions in various action files to directly call getBlock, getBlockNumber, and getChainId functions instead of using getAction.

Detailed summary

  • Replaced getAction calls with direct calls to getBlock, getBlockNumber, and getChainId functions in multiple files.
  • Updated function signatures and parameters accordingly.
  • Removed unnecessary getAction imports from specific files.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

changeset-bot bot commented Aug 17, 2024

⚠️ No Changeset found

Latest commit: 9d97e93

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Aug 17, 2024

@holic is attempting to deploy a commit to the Wevm Team on Vercel.

A member of the Team first needs to authorize it.

@@ -30,7 +30,6 @@ export type EstimateMaxPriorityFeePerGasErrorType =
| GetBlockErrorType
| HexToBigIntErrorType
| RequestErrorType
| GetBlockErrorType
Copy link
Contributor Author

@holic holic Aug 17, 2024

Choose a reason for hiding this comment

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

this was already in the union a few lines above

@@ -52,6 +52,7 @@ export type EstimateFeesPerGasReturnType<

export type EstimateFeesPerGasErrorType =
| BaseFeeScalarErrorType
| GetBlockErrorType
Copy link
Contributor Author

@holic holic Aug 17, 2024

Choose a reason for hiding this comment

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

we call getBlock here, so this error type should also be included here?

Copy link
Contributor

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

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

My worry here is if this pattern is expected, that means third party actions will also need to remember to implement it.

Maybe this is a unimportant because a third party action is less likely to be overriden.

Maybe a solution to that would be to encourage folks to build actions using a defineAction(...) utility or something?

Comment on lines +59 to +62
return getAction(client, _getBlockNumber, 'getBlockNumber')(params)
}

export async function _getBlockNumber<chain extends Chain | undefined>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually necessary to export both? Shouldn't getBlockNumber always "just work"?

Copy link
Contributor Author

@holic holic Aug 18, 2024

Choose a reason for hiding this comment

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

My understanding is if I override getBlockNumber with my own action, then inside that override call into viem's getBlockNumber here, it'd end up resolving to my own getBlockNumber, not viem's underlying one, because that's the one attached to the client object?

That points to a potentially deeper issue though - would this result in an infinite loop?

Comment on lines +96 to +101
>(
client: Client<Transport, chain, account>,
params: GetBlockParameters<includeTransactions, blockTag> = {},
): Promise<GetBlockReturnType<chain, includeTransactions, blockTag>> {
return getAction(client, _getBlock, 'getBlock')(params)
}
Copy link
Contributor

@roninjin10 roninjin10 Aug 18, 2024

Choose a reason for hiding this comment

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

Suggested change
>(
client: Client<Transport, chain, account>,
params: GetBlockParameters<includeTransactions, blockTag> = {},
): Promise<GetBlockReturnType<chain, includeTransactions, blockTag>> {
return getAction(client, _getBlock, 'getBlock')(params)
}
>(
client: Client<Transport, chain, account>,
params: GetBlockParameters<includeTransactions, blockTag> = {},
): Promise<GetBlockReturnType<chain, includeTransactions, blockTag>> {
return getAction(client, _getBlock, 'getBlock')(params)
}

Might be able to do this just to save some boilerplate

const withGetAction = <TAction>(action: TAction, name: string): TAction => {
  return ((client, params) => {
     return getAction(client, action, name)(params)
  }) as unknown as TAction
}

export const getBlock = withGetAction(_getBlock, 'getBlock')

Copy link
Contributor

Choose a reason for hiding this comment

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

DOn't do this it's convenient but will make stack traces suck

@holic
Copy link
Contributor Author

holic commented Aug 18, 2024

means third party actions will also need to remember to implement it

this is already true for getAction today, which isn't correctly used in most cases

@holic
Copy link
Contributor Author

holic commented Aug 18, 2024

Another thing I am realizing that I don't think I fully internalized: viem clients only support _one_ instance of a given action, rather than a stack of an action (like middleware).

So if I have two decorators that each override getChainId, only the most recently used one (called via .extend) will be attached to the client and called by all other actions.

Which makes sense, because the action ultimately calls out to another lower level action or RPC method.

I have a use case where I override writeContract to allow submitting txs to the mempool serially to ensure nonce ordering. In this action, I wanted to internally call whatever the underlying writeContract action is on the client, to preserve original behavior. But that would be my own writeContract if you extended the client with this action, rather than the one attached before it?

Nevermind, it does work like a stack of middleware. Test PR coming!

@jxom jxom force-pushed the main branch 3 times, most recently from b56e162 to ad2831b Compare September 7, 2024 02:46
@jxom
Copy link
Member

jxom commented Oct 14, 2024

closing for now – will revisit later

@jxom jxom closed this Oct 14, 2024
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.

3 participants