move: resolve function names for vm errors

[rvantonder]

Fixes #7291

After chatting with @tnowacki, we go with the approach to first convert

impl From<VMError> for ExecutionError {
    fn from(error: VMError) -> Self { ... } ...}

to take a resolver to resolve function names. It is now:

pub fn convert_vm_error(
    error: VMError,
    fname_resolver: impl FunctionNameResolver, 
) -> ExecutionError { ... }

pub trait FunctionNameResolver {
    fn resolve(&self, m: &ModuleId, idx: FunctionDefinitionIndex) -> Option<Identifier>;                                                
}   

The resolver takes module ID and fn index (which we get from error) to resolve the function name. In adapter.rs the resolver uses vm state to resolve function names (what we want, now function names are resolving, yay) .

In id_leak_verifier.rs we use an empty resolver (doesn't make a difference compared to previous logic). I'm not sure if/how to resolve the function name from this context. @tnowacki may have an idea, since he mentioned:

the only spot that the function could be coming from is the current module we are checking, so we could give an invariant violation if that is nto true. Really for (1) we should revert Sam’s change from last summer that forces the error type there to be VM error, and go back to having an associated type in the abstract interpreter

I feel like we can address that in a follow up PR if it's involved.

I'm not sure why the e2e test is failing, but it doesn't look specific to these changes (I see other open PRs failing on similar things?). I tried to run e2e tests locally without much success (against devnet and local network).

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

4 Ignored Deployments

Name	Status	Preview	Comments	Updated
explorer	⬜️ Ignored (Inspect)			Jan 27, 2023 at 6:30AM (UTC)
explorer-storybook	⬜️ Ignored (Inspect)			Jan 27, 2023 at 6:30AM (UTC)
frenemies	⬜️ Ignored (Inspect)			Jan 27, 2023 at 6:30AM (UTC)
wallet-adapter	⬜️ Ignored (Inspect)			Jan 27, 2023 at 6:30AM (UTC)

[amnn]

I think if we get to a point where the null case doesn't need to be handled (i.e. there really is only one lambda) then I can get behind going back to a lambda, it's just when the lambda type is particularly hairy, or gets passed down multiple levels, that traits edge it out for me in terms of being documentable etc. I agree this case is borderline though, and I don't feel particularly strong about it.

[tnowacki]

I think if we get to a point where the null case doesn't need to be handled (i.e. there really is only one lambda) then I can get behind going back to a lambda, it's just when the lambda type is particularly hairy, or gets passed down multiple levels, that traits edge it out for me in terms of being documentable etc. I agree this case is borderline though, and I don't feel particularly strong about it.

The null case isn't needed even for this PR. Sorry for not enough clarity on that.

In the ID leak verifier, there is only one case: The error comes from a function defined in the current module. So you can always just look up the name of the function in the current module, and if the module doesn't match, give an invariant violation instead (though that should be impossible... like really impossible)

But to make that impossibility more clear, I'd be fine on waiting on this until we can land the update

[rvantonder]

OK update on feedback, please take another look

@tnowacki: I think we should maybe preserve the function index information? No need to hide the index?

• Addressed error formatting. Around preserving index: I think it's useful for a compiler engineer when debugging, and less so for a user (adds noise IMO), but don't feel that strongly about this.

@tnowacki: I don't think the NullResolver is needed [...] I'm also not a big fan of traits when a single function/lambda would do

• I removed need for a NullResolver. Because of that I also made the resolver a lambda again, and no longer the trait suggested by @amnn.

@tnowacki: Is this ever None now? (referring to whether we can resolve the function name to Some value)

• Unfortunately it's difficult to ensure it's never None, because the resolver function does vm.load_module(module_id, ...); which can fail. In order to ensure it's never None, we need to audit all callers will succeed loading a module, i.e., vm.load_module is always Some, and this would be a pretty heavy exercise. I could see restructuring this code into pipelines where we organize and ensure neatly when modules are resolvable, but that's a bigger ask. I think things are OK returning Option<Identifier> for now.

[rvantonder]

Addressed more comments, and made the call to pass vm and state_view to convert_vm_error directly and inline, instead of lambdas/closures. Think we're close now, please take another look @tnowacki 🙏

[rvantonder]

no worries, I don't want things to degrade either, appreciate the diligence!