-
Notifications
You must be signed in to change notification settings - Fork 6
Only use Runtime objects in AsyncSubstrateInterface #141
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
base: staging
Are you sure you want to change the base?
Conversation
…istently using the runtime object or the self.runtime
Still ironing out some issues. Don't review yet. Need it in "ready for review" for tests though. |
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.
Functionally LGTM, but think about comments
module_name, | ||
error_name, | ||
block_hash=None, |
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.
add annotations
self, | ||
module_name, | ||
constant_name, | ||
block_hash=None, |
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.
missed annotations
self, | ||
storage_item: ScaleType, | ||
module, | ||
spec_version_id, |
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.
missed annotations
@pytest.mark.asyncio | ||
async def test_legacy_decoding(): | ||
# roughly 4000 blocks before metadata v15 was added | ||
pre_metadata_v15_block = 3_010_611 | ||
|
||
async with AsyncSubstrateInterface(ARCHIVE_ENTRYPOINT) as substrate: | ||
block_hash = await substrate.get_block_hash(pre_metadata_v15_block) | ||
events = await substrate.get_events(block_hash) | ||
assert isinstance(events, list) |
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.
I'd avoid do real calls within kinda unit tests
bc it's already e2e testing actually
The same for another tests
AsyncSubstrateInterface has a problem. As it was ported from the synchronous version of SubstrateInterface, the
self.runtime
would always be valid in a synchronous context, but in an async context, where you may be concurrently querying multiple blocks with differing runtimes, there are potential issues. As the runtime version does not change often, this is unlikely to occur, but it certainly can, and when it does it's incredibly difficult to locate the source of the problems.This PR addresses those issues by ensuring that we always create Runtime objects, and pass those around, rather than relying on the
self.runtime
to be correct at any given time.The end-users will be completely unaffected negatively, as they do not need to specify Runtimes, but may see some speed improvements in various chained calls that call
AsyncSubstrateInterface.init_runtime
, in addition to better assurance of correctness.The only issue I see may come from subscription handlers. Though I will investigate this further before finishing the PR and setting it R4R.UnaffectedAdded benefit: this made it easy to add in legacy Metadata V14 support, which will resolve #112