X-chain - Repack tx builder #2972
Conversation
| @@ -60,7 +60,10 @@ type FormattedAssetID struct { | |||
| } | |||
|
|
|||
| // Service defines the base service for the asset vm | |||
| type Service struct{ vm *VM } | |||
There was a problem hiding this comment.
most of the code in this files has been:
- Moved to
txs/builder/builder.gofile - replaced by wallet machinery
| @@ -98,35 +95,6 @@ func (w *WalletService) issue(tx *txs.Tx) (ids.ID, error) { | |||
| return txID, nil | |||
| } | |||
|
|
|||
| func (w *WalletService) update(utxos []*avax.UTXO) ([]*avax.UTXO, error) { | |||
There was a problem hiding this comment.
moved to wallet_service_backend.go
| @@ -1555,27 +1555,27 @@ func TestServiceGetTxJSON_OperationTxWithPropertyFxMintOpMultiple(t *testing.T) | |||
| func newAvaxBaseTxWithOutputs(t *testing.T, genesisBytes []byte, chainID ids.ID, fee uint64, parser txs.Parser) *txs.Tx { | |||
| avaxTx := getCreateTxFromGenesisTest(t, genesisBytes, "AVAX") | |||
| key := keys[0] | |||
| tx := buildBaseTx(avaxTx, chainID, fee, key) | |||
| tx := buildTestBaseTx(avaxTx, chainID, fee, key) | |||
There was a problem hiding this comment.
these renamings here avoid conflicts with functions defined in tx_builders.go. We will further simplify this test code in next PR, #2736
| [][]*secp256k1.PrivateKey, // signers | ||
| error, | ||
| ) | ||
|
|
There was a problem hiding this comment.
SpendNFT is the only function I have not dropped, since it's not obvious to me whether is should be moved to the wallet.
Keeping it here for now
| // Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. | ||
| // See the file LICENSE for licensing terms. | ||
|
|
||
| package avm |
There was a problem hiding this comment.
I think the naming for stuff in this file is a bit confusing and suggests that it might be better for us to put this in its own package. Maybe we can make a new package for this component that we can call avm/wallet so we can just call this wallet.Wallet instead of avm.walletServiceBuilderBackend.
There was a problem hiding this comment.
I'd go for the P-chain structure.
In platformvm we used to have a txs/builder package, which now has been renamed to txs/txstest since those builders are only used in tests.
Here I'd keep a txs/builder package as long as these builder are used in the APIs
|
|
||
| var _ txBuilderBackend = (*walletServiceBackend)(nil) | ||
|
|
||
| func NewWalletServiceBackend(vm *VM) *walletServiceBackend { |
There was a problem hiding this comment.
Can we accept an UTXOReader instead of the concrete *VM type here?
There was a problem hiding this comment.
the vm here is used by wallet service as well. I'd keep vm to centralize access
Signed-off-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>
| @@ -0,0 +1,30 @@ | |||
| // Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. | |||
| // See the file LICENSE for licensing terms. | |||
There was a problem hiding this comment.
the file organization of this folder is very similar to what we do in P-chain.
The difference is that in platformvm we don't have multiple backends (wallet and service backends), so we can fully define the backend in the tx builder package.
|
Closing this dow since we reached agreement on not upgrading deprecated X-chain APIs to Dynamic fees. |
Why this should be merged
Dynamic fees will radically chance the way we build transactions. Fees won't be known ahead of time, but will be iterativelly calculated as inputs and outputs are added to the tx.
On top of this we have at least three ways to build transactions currently:
This code duplication is a maintenance nightmare as we introduce dynamic fees.
This PR simplify the maintenance burned in two ways:
In a coming PR, unit tests are update to make use of the txs builders instead of manually laying out txs, so to have a unified and maintainable way to build X-chain txs
How this works
How this was tested
CI