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

feat: cast mktx #7056

Merged
merged 4 commits into from
Mar 1, 2024
Merged

feat: cast mktx #7056

merged 4 commits into from
Mar 1, 2024

Conversation

ay
Copy link
Contributor

@ay ay commented Feb 9, 2024

Motivation

Similar to seth mktx from dapptools, there should be a command to build and sign transactions without broadcasting them. Ideally it should be possible to do this on an offline machine.

Solution

cast mktx is the same as cast send except it prints out a raw hex encoding of the signed transaction instead of broadcasting. This output can then be published with cast publish. It also works without an RPC (for example on an air-gapped machine) if all the required options are set.

I can also update the Foundry book if merged.

Closes #1273

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I'm supportive, since seth also has this.

I'm fine with the verbose build_tx function, because we'll clean this up shortly anyway

could we add a few tests for this

pta closer look @Evalir

Comment on lines +102 to +104
let signature = provider.sign_transaction(&tx, from).await?;
let signed_tx = tx.rlp_signed(&signature);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're actually signing the transaction, this probably should be behind cast wallet similar to the existing cast wallet sign. What do you think about also renaming this to cast wallet sign-tx to make it more explicit? I don't think cast needs to be backwards compatible with seth anymore.

I could imagine a separate cast mktx command that returns a populated (optionally serialized) transaction, without signing it

Copy link
Contributor Author

@ay ay Feb 9, 2024

Choose a reason for hiding this comment

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

I went back and forth a few times on this and initially implemented this as cast wallet sign-tx, but ultimately decided cast mktx is better. The cast wallet commands don't use TxBuilder, don't use an RPC connection, and therefore have no way of filling a transaction with gas info, nonce, ENS resolution, etc. Having it as cast mktx allows it to be as close to cast send as possible, with the difference being it prints the encoded transaction instead of broadcasting it. You get everything provided by TxBuilder if you need it, but can still specify all the options manually if you have no connection to an RPC endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it can be renamed to something like cast sign-tx, but since the behavior is identical to seth mktx (which builds and signs transactions) I went with cast mktx.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a good general principle is to implement the UX that's best for the user, even if it's more work because cast wallet commands don't yet have TxBuilder or RPC connections. Most foundry users haven't used (or heard of) dapptools and seth is no longer maintained, so we should use the best naming/UX regardless of what seth did.

However, IMO the cast UX is already messy and needs breaking change cleanup, so I won't block this PR on trying to keep cast UX clean or changing command names. Just sharing my thoughts, and will defer to you and other foundry maintainers from here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree. I wasn't sure what the best place for this was either. Because of how close its logic is to cast send, I put it adjacent to where that is. I can rename/move it wherever (though moving it under cast wallet will require a few more changes like importing TxBuilder and the utility functions in common with cast send).

@ay
Copy link
Contributor Author

ay commented Feb 9, 2024

I've added some tests.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry for the delay.

lgtm

the tx builder abomination will we refactored with cast alloy migration, so this is fine

@mattsse mattsse merged commit e57e82c into foundry-rs:master Mar 1, 2024
19 checks passed
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.

cast mktx / sign transactions without broadcasting
3 participants