Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P-chain - Tx builder cleanup #2718

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Feb 8, 2024

Why this should be merged

The upcoming work on dynamic fees will change they way we fund P-chain transactions.
This is much easier to be done and reviewed if we use txBuilder package as much as possible in UTs, instead of trying to build txs "manually".
Also we have ideas to eventually make txBuilder a test only package and use the wallet as the way to build and func transactions. This PR simplifies the upcoming changes

How this works

Make utxo.Spend and utxo.Authorized be called only in:

  1. Unit tests
  2. txBuilder package

How this was tested

CI


// Returns a shared memory where GetDatabase returns a database
// where [recipientKey] has a balance of [amt]
fundedSharedMemory := func(peerChain ids.ID, assets map[ids.ID]uint64) atomic.SharedMemory {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this a clean way to setup the shared memory when we want to build an importTx. Pulled the function out of the test to be reused in other tests

@@ -965,44 +987,23 @@ func TestDurangoDisabledTransactions(t *testing.T) {
{
name: "AddValidatorTx",
buildTx: func(env *environment) *txs.Tx {
ins, unstakedOuts, stakedOuts, signers, err := env.utxosHandler.Spend(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I mean by building a tx "manually". The tx is supposed to pass verification, so we really should use the txBuilder to do that. This also shorten the test

},
},
{
name: "ImportTx",
setupTest: func(env *environment) (txs.UnsignedTx, [][]*secp256k1.PrivateKey, state.Diff, *types.JSONByteSlice) {
setupTest: func(env *environment, memoField []byte) (*txs.Tx, state.Diff) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is really the reason why I added memo fields in the txBuilder functions.
With the activation of Durango, we zeroed memo fields, so it may seem weird that I introduce them back.
However txBuilder is going to become a test package and we may want to be able to setup memos in tests.

Copy link
Contributor

@marun marun Feb 8, 2024

Choose a reason for hiding this comment

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

So txtBuilder will become test-only? What would be the use case for using memos in tests? It might be helpful to document your intent, maybe as part of a txBuilder README?

Copy link
Contributor Author

@abi87 abi87 Feb 9, 2024

Choose a reason for hiding this comment

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

Currently memo fields are disabled, but we may consider reintroducing them once we properly price txs (with dynamic fees). Historically they were not as important on the P-chain as they are on the X-chain (where we chose not to zero them).
I think generally we want to allow maximal flexibility in building txs, especially in UTs.
Note for instance that with Durango activation we won't need to specify StartTime in any of the AddValidator/Delegator txs but we still explicitly pass start time in the builder signature.
Not sure I understand what should go in the README here?

ins, unstakedOuts, stakedOuts, signers, err := utxoHandler.Spend(
vm.state,
keys,
primaryTx, err := vm.txBuilder.NewAddPermissionlessValidatorTx(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some more "manually built" txs that we can cleanup

@abi87 abi87 self-assigned this Feb 8, 2024
@abi87 abi87 linked an issue Feb 8, 2024 that may be closed by this pull request
5 tasks
vms/platformvm/txs/builder/builder.go Show resolved Hide resolved
vms/platformvm/txs/builder/builder.go Show resolved Hide resolved
vms/platformvm/txs/builder/builder.go Show resolved Hide resolved
},
},
{
name: "ImportTx",
setupTest: func(env *environment) (txs.UnsignedTx, [][]*secp256k1.PrivateKey, state.Diff, *types.JSONByteSlice) {
setupTest: func(env *environment, memoField []byte) (*txs.Tx, state.Diff) {
Copy link
Contributor

@marun marun Feb 8, 2024

Choose a reason for hiding this comment

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

So txtBuilder will become test-only? What would be the use case for using memos in tests? It might be helpful to document your intent, maybe as part of a txBuilder README?

@abi87 abi87 requested a review from marun February 9, 2024 10:58
@StephenButtolph StephenButtolph added this to the v1.11.0 milestone Feb 12, 2024
@StephenButtolph StephenButtolph added the testing This primarily focuses on testing label Feb 12, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue Feb 12, 2024
Merged via the queue into master with commit 2513581 Feb 12, 2024
17 checks passed
@StephenButtolph StephenButtolph deleted the p-chain_tx-builder-cleanup branch February 12, 2024 19:04
mboben pushed a commit to mboben/avalanchego that referenced this pull request Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing This primarily focuses on testing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

P-chain - Dynamic Fees
3 participants