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

Use generic types instead of trait objects in tx pool #3308

Merged
merged 1 commit into from Apr 30, 2020

Conversation

hashmap
Copy link
Contributor

@hashmap hashmap commented Apr 24, 2020

Tx pool takes some parameters as trait objects. It's not an idiomatic Rust code, in this particular case we should use generic types. Trait object makes sense when we accept in runtime different concrete types which implement the trait as a value of the same field. It's not the case here. Trait objects come with a price - instead of method dispatch in compile time we have to accept runtime dispatch. My guess we did it to not clutter the code with type parameters, which is understandable but still suboptimal.

Basically the change is to replace

pub struct TransactionPool {
        pub blockchain: Arc<dyn BlockChain>,
	pub verifier_cache: Arc<RwLock<dyn VerifierCache>>,
	pub adapter: Arc<dyn PoolAdapter>,
        ...
}

with

pub struct TransactionPool<B, P, V>
where
	B: BlockChain,
	P: PoolAdapter,
	V: VerifierCache,
{
     pub blockchain: Arc<B>,
     pub verifier_cache: Arc<RwLock<V>>,
     pub adapter: Arc<P>,
...
}

The rest is just passing type parameters to Pool and TransactionPool

Tx pool takes some parameters as trait objects. It's not an idiomatic Rust code, in this particular case we should use generic types. Trait object makes sense when we accept in runtime different concrete types which implement the trait as a value of the same field. It's not the case here. Trait objects come with a price - instead of method dispatch in compile time we have to accept runtime dispatch. My guess we did it to not clutter the code with type parameters, which is understandable but still suboptimal.
Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me.

@hashmap hashmap merged commit 9e51e86 into mimblewimble:master Apr 30, 2020
@antiochp antiochp mentioned this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants