sae: Implement custom tx AsOp#5308
Conversation
…ttolph/atomic-tx-rewrite.2
…ttolph/atomic-tx-rewrite.2
|
|
||
| // scaleAVAX converts an amount denominated in nAVAX into the C-Chain's aAVAX | ||
| // denomination. | ||
| func scaleAVAX(nAVAX uint64) uint256.Int { |
There was a problem hiding this comment.
IMO this name should be clear that this is a one way conversion from X-chain to C-chain. I don't have a better name for this, but reading the code doesn't not make it clear the denominations we end up with
There was a problem hiding this comment.
I suppose I was relying on the types to make it clear that the AVAX was being scaled upwards rather than downwards. xAVAXToC feels like a horrible name... toAAVAX also seems gross... bigAVAX could work, although I'm not sure that really addresses your concern
There was a problem hiding this comment.
Not a big enough deal to block merge, and those options are worse... I guess we can keep it
| // Imported for [atomic.TxBytesGas] comment resolution. | ||
| _ "github.com/ava-labs/avalanchego/graft/coreth/plugin/evm/atomic" |
There was a problem hiding this comment.
I don't think you should import this just for the comment resolution. You could write out the path
There was a problem hiding this comment.
Hm, why not? Is the concern about init functions in coreth? I don't think there is anything too concerning in the init functions.. I suppose maybe the concern is around the ext data hashes?
There was a problem hiding this comment.
meh whatever, i just thought it was unnecessary. but we can punt this until we eventually deprecate coreth completely
There was a problem hiding this comment.
I think we've been fine adding imports for comment resolution previously.
There was a problem hiding this comment.
Pull request overview
This PR adds an SAE-focused conversion path from C-Chain atomic transactions into hook.Op by implementing Tx.AsOp, including gas used, effective gas price, and AVAX mint/burn effects that correspond to coreth’s atomic tx accounting.
Changes:
- Add
Tx.AsOp(avaxAssetID)plus supporting gas/fee and AVAX-scaling helpers. - Extend
Import/Exportto implement newUnsignedinterface methods (burned,numSigs,asOp) to generate SAE operations. - Expand unit tests and add a fuzz compatibility test to compare
AsOpbehavior against coreth.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vms/saevm/cchain/tx/tx.go |
Introduces Tx.AsOp and supporting gas/fee calculation helpers. |
vms/saevm/cchain/tx/import.go |
Implements Unsigned methods for imports and generates mint-side hook.Op data. |
vms/saevm/cchain/tx/export.go |
Implements Unsigned methods for exports and generates burn-side hook.Op data. |
vms/saevm/cchain/tx/tx_test.go |
Extends mainnet + synthetic vectors and adds AsOp unit/error tests. |
vms/saevm/cchain/tx/compatibility_test.go |
Adds fuzzing to compare AsOp output vs coreth state transfer. |
vms/saevm/cchain/tx/BUILD.bazel |
Updates Bazel deps for new imports (uint256, hook, gas/math, ap5). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JonathanOppenheimer
left a comment
JonathanOppenheimer
left a comment
There was a problem hiding this comment.
need to go through tests more
| // Imported for [atomic.TxBytesGas] comment resolution. | ||
| _ "github.com/ava-labs/avalanchego/graft/coreth/plugin/evm/atomic" |
There was a problem hiding this comment.
I think we've been fine adding imports for comment resolution previously.
| ID: ids.FromStringOrPanic("h34BPNmYApCbW8buVWAtzu1KtjTFmyMhiRQQnAqPqwCqQsB7f"), | ||
| Bytes: common.FromHex("0x000000000000000000010427d4b22a2a78bcddd456742caf91b56badbff985ee19aef14573e7343fd652ed5f38341e436e5d46e2bb00b45d62ae97d1b050c64bc634ae10626739e35c4b00000001c52b712aa7dce27a650bf509f799673e245edd4fa9e4e1700eb6105202fe579a0000000121e67317cbc4be2aeb00677ad6462778a8f52274b9d605df2591b23027a87dff000000050000000002faf080000000010000000000000001b8b5a87d1c05676f1f966da49151fa54dbe68c330000000002faf08021e67317cbc4be2aeb00677ad6462778a8f52274b9d605df2591b23027a87dff0000000100000009000000013e6614876ee01d3b8b27480c00bdcb0ae84ee3e8346d2d5f08320f7dd3e76c4540be021fe85e91817654c9310b54e8f2e88d81db52b8693842b90f3dbd23bd5c01"), | ||
| Op: hook.Op{ | ||
| ID: ids.FromStringOrPanic("h34BPNmYApCbW8buVWAtzu1KtjTFmyMhiRQQnAqPqwCqQsB7f"), |
There was a problem hiding this comment.
What's the value in hard-coding these per test? Does this catch regressions?
Could we instead hash test.Bytes on the fly,to get the ID with some kind of helper for all of these instances -- I feel like that would be simpler than the current setup.
There was a problem hiding this comment.
I feel like hashing the bytes is just duplicating the code in the test
Why this should be merged
This PR factors out
Tx.AsOpfrom #5303.How this works
Tx.AsOpincludes multiple pieces of critical data for SAE:op.Gas- which correlates toatomic.UnsignedTx.GasUsedin coreth.op.GasFeeCap- which correlates toatomic.EffectiveGasPricein coreth.op.Burnandop.Mint- which correlate to the non-multicoin modifications inatomic.UnsignedTx.EVMStateTransferin coreth.How this was tested
TestAsOp_Errorsto test error conditionsAsOpagainst corethNeed to be documented in RELEASES.md?
No