Skip to content

Txm no longer receives Viem objects#298

Merged
GabrielMartinezRodriguez merged 4 commits into
masterfrom
gabriel/txm-timeout
Jan 7, 2025
Merged

Txm no longer receives Viem objects#298
GabrielMartinezRodriguez merged 4 commits into
masterfrom
gabriel/txm-timeout

Conversation

@GabrielMartinezRodriguez

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Dec 6, 2024

Copy link
Copy Markdown
Contributor

Linked issues

closes https://linear.app/happychain/issue/HAPPY-135/txmanager-add-timeouts-to-rpc-calls

Description

We aimed to configure a timeout for the RPC calls, but to achieve this, we ultimately decided not to expose viem objects in the TXM API. This ensures the TXM remains agnostic of the Web3 library we are using. For more context, please refer to the related issue

Toggle Checklist

Checklist

Basics

  • B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
  • B2. This PR is not so big that it should be split & addresses only one concern.
  • B3. The PR targets the lowest branch it can (ideally master).

Correctness

  • C1. Builds and passes tests.

  • C2. The code is properly parameterized & compatible with different environments.

  • C3. I have manually tested my changes & connected features.

  • C4. I have performed a thorough self-review of my code after submitting the PR.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code.
  • D2. All public-facing APIs & meaningful internal APIs are properly documented.
  • D3. If appropriate, the general architecture of the code is documented.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Dec 6, 2024

Copy link
Copy Markdown

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1b25f8b
Status: ✅  Deploy successful!
Preview URL: https://02f20aa0.happychain.pages.dev
Branch Preview URL: https://gabriel-txm-timeout.happychain.pages.dev

View logs

GabrielMartinezRodriguez commented Dec 6, 2024

Copy link
Copy Markdown
Contributor Author

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title Simplify TransactionManager configuration Txm no longer receives Viem objects Dec 6, 2024
@linear

linear Bot commented Dec 6, 2024

Copy link
Copy Markdown

@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review December 6, 2024 15:09
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label Dec 6, 2024
transport: webSocket(),
chain: anvil,
privateKey: env.PRIVATE_KEY,
chainId: 31337,

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.

suggestion: why dont we put the chainId in the env file?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yup, let's do this!

this.blockTime = _config.blockTime || 2n
this.finalizedTransactionPurgeTime = _config.finalizedTransactionPurgeTime || 2 * 60 * 1000

if ((timeout + retryDelay) * retries > this.blockTime * 1000n) {

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.

maybe its me but this line feels like a lot to parse 😅

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm pretty okay with it. Maybe:

const timePerRetry = timeout + retryDelay
if (timePerRetry * retries > this.blockTime * 1000n) {

Comment on lines +168 to +182
let transport: ViemTransport
if (protocol.value === "http") {
transport = viemHttpTransport(_config.rpc.url, {
timeout,
retryCount: retries,
retryDelay,
})
} else {
transport = viemWebSocketTransport(_config.rpc.url, {
timeout,
retryCount: retries,
retryDelay,
})
}

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.

might be nice to extract this to some utility

const transport = getViemTransport(protocol, _config.rpc)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't mind too much, it's only used here, and this is pretty simple function that is just a long list of things being initialized, so it's not like it pollutes reasoning about what's otherwise a complex flow.

Comment on lines +289 to +300
// Get the chain ID of the RPC node
const rpcChainIdPromise = this.viemClient.safeGetChainId()

// Start the transaction repository
await this.transactionRepository.start()

// Start the nonce manager, which depends on the transaction repository
await this.nonceManager.start()

const rpcChainId = await rpcChainIdPromise

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.

why seperate rpcChainIdPromise like this?

const rpcChainId = await this.viemClient.safeGetChainId()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Latency optimization: kick off the call early, then do the initialization, then come back and wait until the id is there. Otherwise we would have to wait the entire network roundtrip for chainid without doing any useful work locally.

@GabrielMartinezRodriguez GabrielMartinezRodriguez Jan 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do it this way for the reasons @norswap mentions

@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review merge-blocked-after-changes Ready to merge but waiting for downstack, after requested changes are made merge-blocked Ready to merge, waiting for downstack and removed merge-blocked-after-changes Ready to merge but waiting for downstack, after requested changes are made reviewing-2 Ready for, or undergoing final review labels Jan 6, 2025
Base automatically changed from gabriel/flush-mechanism to master January 7, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-blocked Ready to merge, waiting for downstack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants