sae: Implement AtomicRequests on custom txs#5325
Conversation
There was a problem hiding this comment.
Pull request overview
This PR factors out and exposes Tx.AtomicRequests() for SAE custom C-Chain atomic transactions (import/export), aligning the shared-memory request generation with the existing coreth implementation.
Changes:
- Add
AtomicRequests()on*Txand plumb anatomicRequests(txID)method through theUnsignedinterface. - Implement shared-memory request generation for
Import(remove requests) andExport(put requests). - Extend unit tests and add a fuzz compatibility test against the coreth implementation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
vms/saevm/cchain/tx/tx.go |
Adds the exported Tx.AtomicRequests() API and extends the Unsigned interface with atomicRequests(txID). |
vms/saevm/cchain/tx/import.go |
Implements Import.atomicRequests to produce shared-memory remove operations for imported UTXOs. |
vms/saevm/cchain/tx/export.go |
Implements Export.atomicRequests to produce shared-memory put operations for exported UTXOs (including traits). |
vms/saevm/cchain/tx/tx_test.go |
Adds test-vector expectations and a unit test for AtomicRequests(). |
vms/saevm/cchain/tx/compatibility_test.go |
Adds fuzz test validating compatibility with coreth’s AtomicOps() output. |
vms/saevm/cchain/tx/BUILD.bazel |
Adds Bazel deps for //chains/atomic to library and tests. |
go.work.sum |
Updates workspace module sums due to dependency graph changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
powerslider
left a comment
powerslider
left a comment
There was a problem hiding this comment.
LGTM! The only optional addition I can think of is a synthetic export test case with two or more recipient addresses, because the static cases only ever exercise single-address outputs, so the path that attaches multiple address traits to an exported UTXO is currently only validated by the fuzzer (which is still fine).
alarso16
left a comment
alarso16
left a comment
There was a problem hiding this comment.
Lots of commits since my review, but all nits
| Name string | ||
| Old *atomic.Tx | ||
| New *Tx | ||
| JSON string | ||
| Bytes []byte | ||
| Op hook.Op | ||
| AtomicRequestsChainID ids.ID | ||
| AtomicRequests *chainsatomic.Requests |
There was a problem hiding this comment.
I feel like this is getting pretty dense, and could maybe be split into more focused helpers / tables?
There was a problem hiding this comment.
I agree, but this is also the last modification to this Tests vector that I'm planning on making...
There was a problem hiding this comment.
I can't think of anything better... Sooo 
There was a problem hiding this comment.
If someone else is concerned about this in the future, I think it's worth taking another look at. It does make tests harder to reason about and compartmentalize. Not blocking though
Why this should be merged
This PR factors out
Tx.AtomicRequestsfrom #5303.How this works
This function is almost exactly taken from coreth, the implementations are almost identical.
How this was tested
Need to be documented in RELEASES.md?
No