Skip to content

sae: Implement custom tx AsOp#5308

Merged
StephenButtolph merged 111 commits intomasterfrom
StephenButtolph/atomic-tx-rewrite.2
May 2, 2026
Merged

sae: Implement custom tx AsOp#5308
StephenButtolph merged 111 commits intomasterfrom
StephenButtolph/atomic-tx-rewrite.2

Conversation

@StephenButtolph
Copy link
Copy Markdown
Contributor

@StephenButtolph StephenButtolph commented Apr 26, 2026

Why this should be merged

This PR factors out Tx.AsOp from #5303.

How this works

Tx.AsOp includes multiple pieces of critical data for SAE:

  • op.Gas - which correlates to atomic.UnsignedTx.GasUsed in coreth.
  • op.GasFeeCap - which correlates to atomic.EffectiveGasPrice in coreth.
  • op.Burn and op.Mint - which correlate to the non-multicoin modifications in atomic.UnsignedTx.EVMStateTransfer in coreth.

How this was tested

  • Extended existing mainnet tx tests to include ops
  • Extended tx tests to include additional mainnet tx
  • Extended tx tests to include synthetic txs for edge cases
  • Added TestAsOp_Errors to test error conditions
  • Fuzzed AsOp against coreth

Need to be documented in RELEASES.md?

No

Comment thread vms/saevm/cchain/tx/export.go
Comment thread vms/saevm/cchain/tx/export.go
Comment thread vms/saevm/cchain/tx/tx.go

// scaleAVAX converts an amount denominated in nAVAX into the C-Chain's aAVAX
// denomination.
func scaleAVAX(nAVAX uint64) uint256.Int {
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.

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

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.

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

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.

Not a big enough deal to block merge, and those options are worse... I guess we can keep it

Comment thread vms/saevm/cchain/tx/tx.go
Comment on lines +15 to +16
// Imported for [atomic.TxBytesGas] comment resolution.
_ "github.com/ava-labs/avalanchego/graft/coreth/plugin/evm/atomic"
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.

I don't think you should import this just for the comment resolution. You could write out the path

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.

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?

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.

meh whatever, i just thought it was unnecessary. but we can punt this until we eventually deprecate coreth completely

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.

I think we've been fine adding imports for comment resolution previously.

Comment thread vms/saevm/cchain/tx/tx_test.go
Comment thread vms/saevm/cchain/tx/tx_test.go Outdated
Comment thread vms/saevm/cchain/tx/tx_test.go
Comment thread vms/saevm/cchain/tx/tx_test.go
Comment thread vms/saevm/cchain/tx/tx_test.go
Base automatically changed from StephenButtolph/smart-atomic-tx-fuzzing to master April 30, 2026 22:08
Copilot AI review requested due to automatic review settings April 30, 2026 22:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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/Export to implement new Unsigned interface methods (burned, numSigs, asOp) to generate SAE operations.
  • Expand unit tests and add a fuzz compatibility test to compare AsOp behavior 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.

Comment thread vms/saevm/cchain/tx/compatibility_test.go
Copy link
Copy Markdown
Contributor

@JonathanOppenheimer JonathanOppenheimer left a comment

Choose a reason for hiding this comment

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

need to go through tests more

Comment thread vms/saevm/cchain/tx/tx.go
Comment thread vms/saevm/cchain/tx/tx.go
Comment on lines +15 to +16
// Imported for [atomic.TxBytesGas] comment resolution.
_ "github.com/ava-labs/avalanchego/graft/coreth/plugin/evm/atomic"
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.

I think we've been fine adding imports for comment resolution previously.

Comment thread vms/saevm/cchain/tx/compatibility_test.go
Comment thread vms/saevm/cchain/tx/tx.go
Comment thread vms/saevm/cchain/tx/tx.go
Comment thread vms/saevm/cchain/tx/export.go
Comment thread vms/saevm/cchain/tx/tx.go
Comment thread vms/saevm/cchain/tx/tx.go
Comment thread vms/saevm/cchain/tx/export.go
Comment thread vms/saevm/cchain/tx/tx_test.go
Comment thread vms/saevm/cchain/tx/tx_test.go
Comment thread vms/saevm/cchain/tx/tx_test.go Outdated
ID: ids.FromStringOrPanic("h34BPNmYApCbW8buVWAtzu1KtjTFmyMhiRQQnAqPqwCqQsB7f"),
Bytes: common.FromHex("0x000000000000000000010427d4b22a2a78bcddd456742caf91b56badbff985ee19aef14573e7343fd652ed5f38341e436e5d46e2bb00b45d62ae97d1b050c64bc634ae10626739e35c4b00000001c52b712aa7dce27a650bf509f799673e245edd4fa9e4e1700eb6105202fe579a0000000121e67317cbc4be2aeb00677ad6462778a8f52274b9d605df2591b23027a87dff000000050000000002faf080000000010000000000000001b8b5a87d1c05676f1f966da49151fa54dbe68c330000000002faf08021e67317cbc4be2aeb00677ad6462778a8f52274b9d605df2591b23027a87dff0000000100000009000000013e6614876ee01d3b8b27480c00bdcb0ae84ee3e8346d2d5f08320f7dd3e76c4540be021fe85e91817654c9310b54e8f2e88d81db52b8693842b90f3dbd23bd5c01"),
Op: hook.Op{
ID: ids.FromStringOrPanic("h34BPNmYApCbW8buVWAtzu1KtjTFmyMhiRQQnAqPqwCqQsB7f"),
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.

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.

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.

I feel like hashing the bytes is just duplicating the code in the test

@StephenButtolph StephenButtolph added this pull request to the merge queue May 2, 2026
Merged via the queue into master with commit c8a1e07 May 2, 2026
60 checks passed
@StephenButtolph StephenButtolph deleted the StephenButtolph/atomic-tx-rewrite.2 branch May 2, 2026 16:05
@github-project-automation github-project-automation Bot moved this from In Progress 🏗️ to Done 🎉 in avalanchego May 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done 🎉

Development

Successfully merging this pull request may close these issues.

4 participants