-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat(dev-tools): Clean up excess Arc usage in sdk #2217
base: develop
Are you sure you want to change the base?
Conversation
Fixing the iota-sdk issue #2187 leaks outside of our crate. Moving forward with it or not will be at the discretion of the impacted crates owners. |
pub struct TransactionBuilder(Arc<dyn DataReader + Sync + Send>); | ||
#[async_trait] | ||
impl<T: DataReader + Send + Sync + 'static> DataReader for Arc<T> { | ||
async fn get_owned_objects( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change somehow related to the Arc cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was necessary because of the cleanup of one of the methods
Code changed too much
Description of change
The SDK APIs are overusing Arcs and can be simplified. Additionally, some places where dynamic dispatch is used unnecessarily can be replaced with static typing.
Reasoning
The extra
Arc
inIotaClient
is there directly because of the signature of theTransactionBuilder::new
fn, which takes anArc<dyn DataReader>
.DataReader
is implemented onReadApi
and thus this constructor must be called withArc<ReadApi>
.One alternative solution is to use the
ReadApi
directly in the client and wrap it in anArc
to pass to this fn. This would work, but still means that there is a doubleArc
used in theTransactionBuilder
.This PR opts to replace the dynamic dispatch with a static type to avoid these layers of indirection.
Related Issues
Closes #2187