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

Refactoring of getters/setters/offset and adapt fuel-core #716

Merged
merged 71 commits into from
Oct 27, 2022

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Oct 20, 2022

Better to merge #702 first before merging this change because the new version of fuel-vm also has other changes.

Applied changes from new fuel-tx and fuel-vm:

  • Usage of Checked<Tx> type instead of CheckedTransaction.
  • Usage of corresponding renamed methods.
  • Replaced the work with public fields by getters and setters.
  • Moved common logic(for Create and Mint) into generic methods.
  • Replaced Interpreter<Database> with Interpreter<Database, Tx> and Interpreter<Database, Script> accordingly to the logic around(See ConcreteStorage).

Added a new PoolTransaction type that describes transactions stored and accepted by the TxPool. It is prepared to support Mint transactions that should be excluded from TxPool. Another solved TODO by this change - store only checked transactions in the TxPool and pass them into Producer(because PoolTransaction stores only checked transactions inside).

The TransactionBuilder from fuel-vm and fuel-tx now generate transaction of exact type as Script or Create instead of Transaction. Almost all tests in the fuel-core prefer to work with the Transaction type, so there is a lot of conversion Script/Create into Transaction.

Close #513

@xgreenx xgreenx added the breaking A breaking api change label Oct 20, 2022
@xgreenx xgreenx self-assigned this Oct 20, 2022
xgreenx and others added 8 commits October 20, 2022 21:34
…eature/methods-refactoring-2

# Conflicts:
#	fuel-block-producer/src/block_producer.rs
#	fuel-block-producer/src/ports.rs
#	fuel-core-interfaces/src/executor.rs
#	fuel-core/src/executor.rs
#	fuel-core/src/schema/tx.rs
#	fuel-tests/tests/messages.rs
#	fuel-tests/tests/tx.rs
#	fuel-tests/tests/tx/utxo_validation.rs
#	fuel-txpool/src/txpool.rs
…ing-2

# Conflicts:
#	fuel-tests/tests/messages.rs
#	fuel-tests/tests/metrics.rs
#	fuel-tests/tests/relayer.rs
#	fuel-tests/tests/tx.rs
#	fuel-tests/tests/tx/predicates.rs
#	fuel-tests/tests/tx/utxo_validation.rs
#	fuel-tests/tests/tx_gossip.rs
// is added into the `fuel_tx::Transaction` enum.
//
// If you face a compilation error, please update the code above and add a new variant below.
match tx {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

leviathanbeak
leviathanbeak previously approved these changes Oct 25, 2022
Copy link
Contributor

@leviathanbeak leviathanbeak left a comment

Choose a reason for hiding this comment

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

left few minor comments, looks solid

@Voxelot
Copy link
Member

Voxelot commented Oct 26, 2022

should we just combine this PR with the mint PR now? Seems like we won't be able to really merge them separately since the types were released together in fuel-tx/fuel-vm.

@xgreenx
Copy link
Collaborator Author

xgreenx commented Oct 26, 2022

should we just combine this PR with the mint PR now? Seems like we won't be able to really merge them separately since the types were released together in fuel-tx/fuel-vm.

We can combine them to make it easy for me=D But I also can create temporary branches in fuel-vm and fuel-tx and use them during the review process

# Conflicts:
#	Cargo.lock
#	fuel-client/Cargo.toml
#	fuel-client/src/client/schema/tx/transparent_receipt.rs
#	fuel-client/src/client/schema/tx/transparent_tx.rs
#	fuel-core-interfaces/Cargo.toml
#	fuel-core/src/executor.rs
#	fuel-core/src/schema/tx/types.rs
#	fuel-tests/tests/tx/utxo_validation.rs
@xgreenx
Copy link
Collaborator Author

xgreenx commented Oct 26, 2022

should we just combine this PR with the mint PR now? Seems like we won't be able to really merge them separately since the types were released together in fuel-tx/fuel-vm.

We can combine them to make it easy for me=D But I also can create temporary branches in fuel-vm and fuel-tx and use them during the review process

I moved the implementation of the coinbase logic into a separate PR #726. This PR contains refactoring with a dummy implementation of Mint.

@@ -130,15 +117,17 @@ impl BlockProducer for MockBlockProducer {
}
}

// TODO: The same code is in the `adapters::transaction_selector::select_transactions`. We need
// to move transaction selection logic into `TxPool` to avoid duplication of the code in tests.
Copy link
Member

Choose a reason for hiding this comment

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

It may make more sense to abstract these fns as a separate module, since these tests use a mocked tx pool and shouldn't take a direct dep on the txpool crate.

Copy link
Member

@Voxelot Voxelot left a comment

Choose a reason for hiding this comment

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

for such a drastic internal structure change to the txs, the breaking impact here actually seems quite minimal. Nice work!

@xgreenx xgreenx requested a review from freesig October 27, 2022 00:20
Copy link
Contributor

@freesig freesig left a comment

Choose a reason for hiding this comment

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

Nice, will be good to get this merged asap

@xgreenx xgreenx merged commit 19d7e3d into master Oct 27, 2022
@xgreenx xgreenx deleted the feature/methods-refactoring-2 branch October 27, 2022 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorporate CheckedTransaction structure into txpool
5 participants