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

Keep more verbose information about each extrinsic #60

Closed
insipx opened this issue Oct 27, 2021 · 13 comments · Fixed by #79
Closed

Keep more verbose information about each extrinsic #60

insipx opened this issue Oct 27, 2021 · 13 comments · Fixed by #79

Comments

@insipx
Copy link
Contributor

insipx commented Oct 27, 2021

A way to show the tree of calls along with argument names, type names, and docs for each argument. The calls can be nested (if you use batch or proxy) so that the full tree might be rather large. But the goal is to basically give someone a lot of confidence that the thing they plan to sign is in fact what they intended to do.

Originally posted by @Noch3 in #58 (comment)

@jsdw would be good to get your input

@jsdw
Copy link
Collaborator

jsdw commented Oct 28, 2021

This definitely sounds useful! I'd lean towards fairly directly exposing the scale-info types I think, so that we don't need to copy a bunch of data out of the registry, and all of the information stored away is accessible!

Potentially we could wrap the types returned to make things a touch more ergonomic, but if we rely more directly on scale-info we can keep the API here fairly slim, too?

@jsdw
Copy link
Collaborator

jsdw commented Nov 3, 2021

One possibility here is to make Value generic over some arbitrary context type T, which is some value that would be stored alongside each value, and to have our decoding set T to TypeId, so that one can query the type information of any value.

I think that the context would probably have to be entirely ignored when trying to deserialize from Value into some concrete type, which would likely mean it would be lost when partially deserializing into some type that contains another Value (eg going from Value to struct Foo { a: String, b: Value }). This could be a little untidy.

Alternately, #66 exposes the type information associated with a given call, and so with that you'd still have a separate tree of types that can be matched up against the tree of Values that have been decoded. This has the advantage of not "polluting" the Value type with any additional complexity, and letting it remain pure to it's purpose of being a decoded value that doesn't care about type information (and is compatible with whatever shapes make sense).

@elliot-u410
Copy link

elliot-u410 commented Nov 3, 2021

"Zipping" together the parsed value with the type information seems like it would be mildly non-trivial, especially since Value is lossy (it encodes multiple metadata types as the same Value constructors IIRC). What you could do, however, is parameterize Value on the type information, as you suggested, but allow for that parameter to be unit (()) indicating that there is no type information. The deserializer would have to use that.

@elliot-u410
Copy link

As an aside, what is the value of deserializing a Value without any metadata context? You would still need to zip the two together to re-encode that deserialized value as an extrinsic/storage value of any sort.

@jsdw
Copy link
Collaborator

jsdw commented Nov 3, 2021

"Zipping" together the parsed value with the type information seems like it would be mildly non-trivial, especially since Value is lossy (it encodes multiple metadata types as the same Value constructors IIRC).

Value is lossy in the sense that you don't know which type it came from, but given a separate type tree, it doesn't seem offhand that it would be too difficult to recurse through the types and values in tandem and line things up (if the type is a tuple, array or composite for instance, the Value should be composite and have the expected length, and if the type is a variant then the Value encodes enough to know exactly which variant it is).

I think it would be useful to experiment with the type info exposed in #66 and see whether it proves to be a pain or not to decode. If I have time, I'll try and construct a non-trivial extrinsic and see how it pans out; it might prove to be useful example code in any case!

What you could do, however, is parameterize Value on the type information, as you suggested, but allow for that parameter to be unit (()) indicating that there is no type information. The deserializer would have to use that.

Yup, definitely! That was precisely my thought (or allowing any T: Default to be deserialized to and just instantiating the default, but one may be surprised by the results here, so () is probably safer). If the above doesn't pan out, it means that two trees are a pain, and something like this feels like the obvious thing to try next :)

As an aside, what is the value of deserializing a Value without any metadata context? You would still need to zip the two together to re-encode that deserialized value as an extrinsic/storage value of any sort.

My thinking here is that Value is simply the minimal generic representation of some decoded data. If you know a little more about the shape you actually expect, you can attempt to deserialize into that shape, and if you know what type it's supposed to be then you can assert that the Value does or does not line up with that type. Otherwise, it doesn't particularly care how it was originally encoded (eg was compact encoding used or not..).

There's something I like about keeping the type and value information distinct and disconnected, so long as given both, one is able to attempt proper encoding. But I can see the advantages of having type information alongside it, too!

Definitely food for thought :)

@elliot-u410
Copy link

elliot-u410 commented Nov 3, 2021

If the "zipping" function were part of desub then it would sort of be six of one, half-dozen of the other. The nice thing about having an "annotated Value" type is that now your zipping function doesn't have to be higher-order.

zip(Metadata, Value, callback: Fn(Value, Type) -> A) -> ??

Now I have to think about how to reduce this callback output. I could provide a reduce function, or assume it comes out as a vector.

But the most obvious output of such a function is that it is also a tree of the same shape as the input:

zip(Metadata, Value<()>, callback: Fn(Value, Type) -> A) -> Value<A>.

At this point you might as well just write zip(Metadata, Value<()>) -> Value<TypeInfo> and then the user can recurse over that.

@jsdw
Copy link
Collaborator

jsdw commented Nov 4, 2021

I think I can get behind a Value<T> type. If we went that route, I'd propose that:

  • Decoding would return Value<TypeId> so that you could look up the type info for a given value in the metadata.
  • Only Value<()> would implement Deserialize
  • Value<T> would implement Deserializer, so that you can deserialize from any Value<T> into a Value<()> (and it's clear that context information is lost in doing so, which offhand I don't think can be avoided when deserializing).
  • Value<T> should perhaps have a without_context(self) method which turns it into a Value<()>. At the very least this would be useful for comparing actual/expected values in tests (the alternative is a PartialEq impl that ignores the context but I think that would be odd/surprising).

@elliot-u410
Copy link

Good thoughts! You won't need to worry about PartialEq because Value<()> and Value<TypeId> will be different types and thus incomparable.

@elliot-u410
Copy link

If all you're carrying is TypeId couldn't that also have Deserialize with one extra field for type information? The structure would be different to accommodate that though.

@jsdw
Copy link
Collaborator

jsdw commented Nov 4, 2021

Yeah, I could deserialize whatever is in the context, but in most cases (like trying to deserialize a Value to a primitive/vec/enum) it would get in the way; the visitor impls for those types would reject an attempt to provide a struct with, say, a "context" and "value" field, for instance.

The other wrinkle is that the type T would have to implement Deserializer or IntoDeserializer or something, which is unlikely.

So, I suspect it would be cleaner to keep the deserializing much as it is and just ignore the context completely.

@elliot-u410
Copy link

I guess I was thinking of just serializing an integer (TypeId) but I'm not sure that's really useful. If this ends up being a zip process then it won't matter if the type IDs are serialized. I personally don't care too much about it because I will always use the scale encoding as the serialization format instead.

@elliot-u410
Copy link

Which reminds me... what prevents desub from becoming sedesub? 😁

@jsdw
Copy link
Collaborator

jsdw commented Nov 4, 2021

Which reminds me... what prevents desub from becoming sedesub? 😁

Only time :)

Ultimately I'd like a single crate to handle decoding and encoding for sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants