[Feat] change-evm-tx-logic#84
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the EVM relaying flow so that signers (local and FKMS-backed remote) receive structured transaction parameters and return a fully signed, EIP-2718 encoded transaction blob, shifting transaction construction/signing responsibilities into the signer/KMS layer.
Changes:
- Introduces an EVM
SignerPayload(JSON) model used to request a fully signed tx blob from both local and remote signers. - Refactors
EVMChainProvider.createAndSignRelayTxto buildSignerPayload, include TSS fields, callSigner.Sign, and unmarshal the signed tx bytes. - Updates FKMS
SignEvmproto RPC/messages to accept anEvmSignerPayload+Tssand returntx_blob.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| relayer/wallet/evm/types.go | Adds SignerPayload struct + constructor for JSON-based signing inputs. |
| relayer/wallet/evm/local_signer.go | Changes local signer to build/sign legacy or EIP-1559 txs and return encoded signed tx bytes. |
| relayer/wallet/evm/remote_signer.go | Changes remote signer to unmarshal JSON payload and call FKMS SignEvm to get TxBlob. |
| relayer/wallet/evm/remote_signer_test.go | Adds a unit test covering the new remote Sign request/response mapping. |
| relayer/chains/evm/provider.go | Refactors relay tx creation/signing to delegate tx construction/signing to the signer via JSON payload. |
| proto/fkms/v1/signer.proto | Redefines SignEvm request/response to take structured fields and return signed tx bytes. |
| proto/fkms/v1/signer.pb.go | Regenerates protobuf Go bindings for the updated schema. |
| proto/fkms/v1/signer_grpc.pb.go | Regenerates gRPC Go bindings for the updated schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r *RemoteSigner) Sign(payload []byte, tssPayload wallet.TssPayload) ([]byte, error) { | ||
| var signerPayload SignerPayload | ||
| if err := json.Unmarshal(payload, &signerPayload); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| res, err := r.FkmsClient.SignEvm( | ||
| r.ContextWithKey(), | ||
| &fkmsv1.SignEvmRequest{Address: strings.ToLower(r.Address), Message: payload}, | ||
| &fkmsv1.SignEvmRequest{ | ||
| SignerPayload: &fkmsv1.EvmSignerPayload{ | ||
| Address: signerPayload.Address, | ||
| ChainId: signerPayload.ChainID, | ||
| Nonce: signerPayload.Nonce, | ||
| To: signerPayload.To, | ||
| GasLimit: signerPayload.GasLimit, | ||
| GasPrice: signerPayload.GasPrice, | ||
| GasFeeCap: signerPayload.GasFeeCap, | ||
| GasTipCap: signerPayload.GasTipCap, | ||
| }, |
There was a problem hiding this comment.
RemoteSigner.Sign currently forwards signerPayload.Address from the JSON payload to FKMS. This bypasses the RemoteSigner’s configured address (r.Address) and allows callers to request signing for a different address than the signer was constructed with. Consider overriding the request address with r.Address, or at least validating (case-insensitive) that signerPayload.Address matches r.Address and returning an error on mismatch.
| signer.GetAddress(), | ||
| cp.Config.ChainID, | ||
| nonce, | ||
| cp.TunnelRouterAddress.Hex(), |
There was a problem hiding this comment.
SignerPayload.Address is populated from signer.GetAddress(), which for geth-derived addresses is usually EIP-55 checksummed (mixed-case). FKMS’s EvmSignerPayload explicitly documents lowercase hex addresses, and the RemoteSigner previously lowercased the address. Normalize the address before marshaling (and/or in walletevm.NewSignerPayload) to avoid FKMS-side validation failures.
| signer.GetAddress(), | |
| cp.Config.ChainID, | |
| nonce, | |
| cp.TunnelRouterAddress.Hex(), | |
| strings.ToLower(signer.GetAddress()), | |
| cp.Config.ChainID, | |
| nonce, | |
| strings.ToLower(cp.TunnelRouterAddress.Hex()), |
| &fkmsv1.SignEvmRequest{ | ||
| SignerPayload: &fkmsv1.EvmSignerPayload{ | ||
| Address: signerPayload.Address, | ||
| ChainId: signerPayload.ChainID, | ||
| Nonce: signerPayload.Nonce, | ||
| To: signerPayload.To, | ||
| GasLimit: signerPayload.GasLimit, |
There was a problem hiding this comment.
proto/fkms/v1/signer.proto documents EVM signer addresses as lowercase hex, but this code sends signerPayload.Address as-is. Since geth’s Address.Hex()/String() is typically EIP-55 checksummed (mixed-case), this can violate the FKMS contract and break signing. Normalize the address (e.g., strings.ToLower(common.HexToAddress(...).Hex())) before populating the FKMS request.
| // Sign unmarshals payload as a SignerPayload JSON, builds and signs the transaction locally, | ||
| // and returns the EIP-2718 encoded signed transaction bytes. | ||
| func (l *LocalSigner) Sign(payload []byte, _ wallet.TssPayload) ([]byte, error) { | ||
| hash := crypto.Keccak256(payload) | ||
| var sp SignerPayload | ||
| if err := json.Unmarshal(payload, &sp); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return crypto.Sign(hash, l.privateKey) | ||
| chainID := new(big.Int).SetUint64(sp.ChainID) | ||
| to := gethcommon.HexToAddress(sp.To) | ||
| value := decimal.NewFromInt(0).BigInt() | ||
|
|
||
| var tx *gethtypes.Transaction | ||
| switch { | ||
| case len(sp.GasPrice) > 0: | ||
| tx = gethtypes.NewTx(&gethtypes.LegacyTx{ | ||
| Nonce: sp.Nonce, | ||
| To: &to, | ||
| Value: value, | ||
| Gas: sp.GasLimit, | ||
| GasPrice: new(big.Int).SetBytes(sp.GasPrice), | ||
| Data: sp.Data, | ||
| }) | ||
| case len(sp.GasFeeCap) > 0: | ||
| tx = gethtypes.NewTx(&gethtypes.DynamicFeeTx{ | ||
| ChainID: chainID, | ||
| Nonce: sp.Nonce, | ||
| To: &to, | ||
| Value: value, | ||
| Gas: sp.GasLimit, | ||
| GasFeeCap: new(big.Int).SetBytes(sp.GasFeeCap), | ||
| GasTipCap: new(big.Int).SetBytes(sp.GasTipCap), | ||
| Data: sp.Data, | ||
| }) | ||
| default: | ||
| return nil, fmt.Errorf("signer payload has neither GasPrice nor GasFeeCap") | ||
| } | ||
|
|
||
| signedTx, err := l.SignTx(tx, chainID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return signedTx.MarshalBinary() | ||
| } |
There was a problem hiding this comment.
The signing behavior in LocalSigner.Sign/SignTx was changed to build and return full EIP-2718 encoded signed transactions (legacy + EIP-1559). There are currently no unit tests asserting the produced tx bytes can be unmarshaled and that the recovered sender matches the local key for both gas modes. Adding tests for these cases would help prevent subtle RLP/signing regressions.
| calldata, err := cp.CreateCalldata(packet) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create calldata: %w", err) | ||
| } | ||
|
|
||
| tx, err := cp.NewRelayTx(ctx, calldata, signer, gasInfo) | ||
| signing, err := chains.SelectSigning(packet) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to select signing: %w", err) | ||
| } | ||
| rAddress, signature := chains.ExtractEVMSignature(signing.EVMSignature) | ||
| tssPayload := wallet.TssPayload{ | ||
| TssMessage: signing.Message, | ||
| RandomAddr: rAddress, | ||
| Signature: signature, | ||
| } |
There was a problem hiding this comment.
createAndSignRelayTx calls CreateCalldata(packet) and then immediately calls chains.SelectSigning(packet)/ExtractEVMSignature again to build the TSS payload. This duplicates packet parsing work and risks divergence if CreateCalldata logic changes independently. Consider returning the needed signing/TSS fields from CreateCalldata (or factoring a shared helper) so they are derived once.
Fixed: #XXXX
Implementation details
let falcon send tx field to fkms and let it create and sign tx
Please ensure the following requirements are met before submitting a pull request:
CHANGELOG_UNRELEASED.mdFiles changedtab in the Github PR explorer)