-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Remove required electrum resolver #23
Conversation
src/runtime.rs
Outdated
} | ||
|
||
pub fn import_contract( | ||
&mut self, | ||
contract: Contract, | ||
resolver: &mut BlockchainResolver, |
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.
Do we really need to take the resolver as a mutable reference? By looking to the code I cannot see where there's the need to change the struct. Making it immutable it would simplify its usage.
Same question applies to all runtime methods that take the resolver as a mutable reference
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.
The reason is that I assume resolver can be a caching resolver, significantly improving validation performance. I mean for the first time the resolver is asked for certain tx it goes to electrum/esplora, caches the answer - and the next time it uses the cache. Such a resolver need to be mutable.
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.
So right now it's not necessary but in the near future it will be, therefore it makes sense defining it as a mutable reference right away
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, since otherwise it will be hard to adopt change to mutable type downstream in the future - and introduction of a simple caching resolver (which is trivial to do as a wrapper type and is not a breaking change) will become a breaking change requiring major version increase.
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.
ACK b0ccaa1
Closes #22