sae: Implement Tx.TransferNonAVAX#5329
Conversation
…ph/atomic-tx-rewrite.4
…ph/atomic-tx-rewrite.4
…labs/avalanchego into StephenButtolph/atomic-tx-rewrite.4
powerslider
left a comment
There was a problem hiding this comment.
LGTM! Non-AVAX transfers now have their own dedicated method, with a fuzz test diffing the full SAE flow against coreth's reference. Nothing else to suggest as improvements.
…ph/atomic-tx-rewrite.4
| // Imported for [atomic.UnsignedExportTx.Burned] comment resolution. | ||
| _ "github.com/ava-labs/avalanchego/graft/coreth/plugin/evm/atomic" | ||
|
|
||
| "github.com/ava-labs/avalanchego/graft/coreth/core/extstate" |
There was a problem hiding this comment.
I thought we weren't import graft?
There was a problem hiding this comment.
Because of the transition logic, we need to share the libevm registrations during the SAE migration.
We can either:
- Have SAE depend on coreth for the libevm registrations
- Move the libevm registrations into SAE and have coreth depend on SAE.
I felt like (1) was less work and wouldn't require moving anything non-SAE related into the SAE codebase (since I presume there is some sprawl in these areas that will be removed once we are in an SAE only world).
But you're right that we should be extremely conservative with any dependencies on coreth. I think it should only be for the libevm registrations.
There was a problem hiding this comment.
This seems like a slippery slope and I don't like importing graft -- I feel like we could get lazy with this. Can we have an SAE package that will be explicitly deprecated and must be removed for things to be 'good', in a similar way we did the RPC stubs and temporary package.
There was a problem hiding this comment.
The options here are either to just start working on the migration of those packages or just to decouple with port interfaces. We will inevitably hit these issues so we just need to decide if we will lean on dependency inversion (at least temporary) or just accept importing from graft.
Here is an example of how I've applied this pattern in graft/evm when I did not want to import any graft/coreth packages in the code.
- Port interface in
graft/evm: https://github.com/ava-labs/avalanchego/blob/master/graft/evm/sync/engine/client.go#L69 - Adapter part in
graft/coreth: https://github.com/ava-labs/avalanchego/blob/master/graft/coreth/plugin/evm/sync_adapter.go
This way my only dependency is the interface while the implementation sits in graft/coreth.
There was a problem hiding this comment.
I don't think it would make sense to try to add some interface indirection. That really only works if you want to own the top and bottom of the stack, but not the middle... For SAE, we want to own the top (but not the bottom) of the stack... So we would need to import graft anyways in that world.
I agree that importing graft is something we need to do very sparingly, but I don't think adding a file with a bunch of aliases really helps us that much... It'll be very easy to see where graft is used (by just searching for the imports) and we will need to inspect the usage as we go around whether or not it makes sense to actually depend on that component within graft.
…ph/atomic-tx-rewrite.4
There was a problem hiding this comment.
I haven't read #5303, or the next in the series so some of this may be irrelevant.
Why this should be merged
This PR factors out the non-AVAX ANT asset transfers from #5303.
How this works
In coreth, all state changes lived under a single
EVMStateTransferfunction. In SAE, we need to split this up into:AsOpalready introduced (1), so this PR adds (2). For compatibility, this PR fuzzes againstEVMStateTransferandAsOp+TransferNonAVAX.How this was tested
Need to be documented in RELEASES.md?
No