feat(zetaclient): add support for EIP-1559 outbound transactions #2387
Conversation
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (23)
You can disable this status message by setting the Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2387 +/- ##
===========================================
+ Coverage 69.10% 69.13% +0.02%
===========================================
Files 308 309 +1
Lines 19221 19307 +86
===========================================
+ Hits 13283 13347 +64
- Misses 5267 5288 +21
- Partials 671 672 +1
|
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis and
ws4charlie
as code owners
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 11033143 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/local/accounts.go | View secret |
| 11033143 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/localnet.yml | View secret |
| 11033143 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/local.yml | View secret |
| 11033144 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/localnet.yml | View secret |
| 11033144 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/local/accounts.go | View secret |
| 11033145 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/local/accounts.go | View secret |
| 11033145 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/localnet.yml | View secret |
| 11033146 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/local/accounts.go | View secret |
| 11033146 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/localnet.yml | View secret |
| 11478175 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/localnet.yml | View secret |
| 11478176 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/config/localnet.yml | View secret |
| 11759679 | Triggered | Generic High Entropy Secret | 3bc0394 | cmd/zetae2e/local/accounts.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
@coderabbitai review |
Actions performedReview triggered.
|
| if err != nil || len(cctxIndex) != 32 { | ||
| return nil, true, fmt.Errorf("decode CCTX %s error", cctx.Index) | ||
| // Determine gas fees | ||
| gas, err := makeGasFromCCTX(cctx, evmObserver.GetPriorityGasFee(), logger) |
There was a problem hiding this comment.
If the priorityFee being used here comes from zetaclients own fetched value thru RPC SuggestGasTipCap. The values may vary across across the group of TSS signers (the external signers are using their own Ethereum node/endpoint, and these nodes can go slightly out-of-sync). In this case, the tx body being built across TSS signers will also vary and lead to TSS keysign failure (because they're signing different transaction bodies).
The key point to achieve EIP-1559 is to be able to use a deterministic GasTipCap value across all TSS signers to sign the transaction. So the cctx struct itself will need to carry a consensus-based GasTipCap, with zetaclients just use the same value to create the tx and then process TSS keysign.
There was a problem hiding this comment.
Good point on this. We will have to post some information on ZetaCore finally.
We can add a new field in the MsgVoteGasPrice and in the GasPrice structure in the crosschain module.
(We could think about renaming the GasPrice structure in the future but right now we could keep the name and just add new fields)
Unlike the actual gas price, here we would not need to interact with the smart contract like fungibleKeeper.SetGasPrice. We would just need to update the structure at the Cosmos module level, it will then be read by all ZetaClient.
I think we can keep this PR open, work on a new one for the gas tip posting, then update this one.
WDYT @swift1337
There was a problem hiding this comment.
Makes total sense for me, thanks for spotting this, @ws4charlie.
| @@ -17,14 +17,14 @@ const ( | |||
|
|
|||
| // Logger contains the base loggers | |||
| type Logger struct { | |||
| Std zerolog.Logger | |||
| zerolog.Logger | |||
There was a problem hiding this comment.
nit: is there any reason we use zerolog instead of the standard library slog? Maybe we could start that conversation on a parallel discussion to eventually migrate and reduce the amount of dependencies on the core node.
| // use memory db if specified | ||
| if strings.Contains(dbPath, ":memory:") { | ||
| path = dbPath | ||
| dial = sqlite.Open(fmt.Sprintf("%s/%s", dbPath, dbName)) |
There was a problem hiding this comment.
nit: Consider moving this statement to a function that standardizes how we expect the db paths.
That way we make this more reusable and deterministic.
| dial = sqlite.Open(fmt.Sprintf("%s/%s", dbPath, dbName)) | |
| dial = sqlite.Open(buildDBPath(dbPath, dbName)) |
| @@ -276,7 +276,7 @@ func TestOpenCloseDB(t *testing.T) { | |||
| }) | |||
| t.Run("should return error on invalid db path", func(t *testing.T) { | |||
| err := ob.OpenDB("/invalid/123db", "") | |||
| require.ErrorContains(t, err, "error creating db path") | |||
| require.ErrorContains(t, err, "unable to ensure dbPath") | |||
There was a problem hiding this comment.
"unable to ensure dbPath" is used a few times across the codebase, consider creating a const for it and reuse whenever needed.
| ob.priorityGasFee = fee | ||
| } | ||
|
|
||
| // GetPriorityGasFee a.k.a. tx.gasTipCap> |
There was a problem hiding this comment.
| // GetPriorityGasFee a.k.a. tx.gasTipCap> | |
| // GetPriorityGasFee returns gasTipCap (EIP-1559) |
|
|
||
| maxFee, ok := new(big.Int).SetString(outboundParams.GasPrice, 10) | ||
| if !ok { | ||
| return Gas{}, errors.New("unable to parse gasPrice from " + outboundParams.GasPrice) |
There was a problem hiding this comment.
nit: consider using fmt.Errorf instead of concatenating strings.
There was a problem hiding this comment.
Yes this is a clear example of where fmt.Errorf is the more correct choice
|
|
||
| // makeGasFromCCTX creates Gas struct based from CCTX and priorityFee. | ||
| func makeGasFromCCTX(cctx *types.CrossChainTx, priorityFee *big.Int, logger zerolog.Logger) (Gas, error) { | ||
| if priorityFee == nil { |
There was a problem hiding this comment.
big.Int in golang can be negative, we should consider it as well and error out.
| cctx3.GetOutboundParams()[0].GasLimit = 2_000_000 | ||
| cctx3.GetOutboundParams()[0].GasPrice = gwei(3).String() | ||
|
|
||
| for _, tt := range []struct { |
There was a problem hiding this comment.
Some other cases would be helpful, such as cases where priorityFee is null, negative, etc.
|
|
||
| // determineDestination picks the destination address and Chain ID based on the status of the cross chain tx. | ||
| // returns true if transaction should be skipped. | ||
| func determineDestination(cctx *types.CrossChainTx, logger zerolog.Logger) (ethcommon.Address, *big.Int, bool) { |
There was a problem hiding this comment.
cctx can be null here, could we introduce a check to have a defensive approach on this func?
| } | ||
|
|
||
| func getCCTXIndex(cctx *types.CrossChainTx) ([32]byte, error) { | ||
| cctxIndexSlice, err := hex.DecodeString(cctx.Index[2:]) // remove the leading 0x |
There was a problem hiding this comment.
This would probably panic (depending how DecodeString is implemented) if cctx is null. Consider adding a null check before this.
|
This PR is obsolete. I'll create a new one. |
Description
This PR introduces support for EIP-1559 gas fees for outbound transactions in zetaclientd.
Closes: #1952, #1085
The main changes are located in:
zetaclient/chains/evm/observer/observer.gozetaclient/chains/evm/signer/gas.gozetaclient/chains/evm/signer/outbound_data.gozetaclient/chains/evm/signer/signer.goChanges
Orchestratorsigner.NewOutboundData(...)DynamicFeeTxGasTipCap(evm.SuggestGasTipCap)Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Checklist: