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

Add codec method to retrieve a Go type registered name #316

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gsora
Copy link

@gsora gsora commented May 12, 2020

Given a codec, users might want to know what's the Amino type associated
with a given struct instance.

This commit adds the functionality needed to do that, for both instance and pointer-to-instance
types.

Nil interfaces are rejected by default.

Method completed with tests.


We are in the process of writing various SDKs for our project (commercionetwork), and for the Go one we thought it might be a great idea to simply reuse all the types implemented in our modules.

We need to know at runtime Amino message types for every possible type in order to construct a well-defined Cosmos REST transaction message.

Right now, we manually parse codec.PrintTypes() output and build a map which associates Go type to its Amino name.

This patch integrates basically the same idea, by leveraging the codec's integrated type map.

gsora added 2 commits May 12, 2020 18:50
Given a codec, users might want to know what's the Amino type associated
with a given struct instance.

This commit adds the functionality needed to do that, for both instance and pointer-to-instance
types.

Nil interfaces are rejected by default.

Method completed with tests.
@alessio alessio requested review from tac0turtle and liamsi May 13, 2020 09:19
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Usually users search for the types and codecs in the relevant code base currently (e.g. tendermint/cosmos-sdk). Maybe it's cool to have a simple way look them up instead 👍


// if i is a non-nil pointer, dereference it and grab its inner Elem
if gt.Kind() == reflect.Ptr {
gt = gt.Elem()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this a **SomeType? (pointer to pointer)

Copy link
Author

@gsora gsora May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, **SomeType would return error even though the struct type has been registered previously.

I could add a check on the inner gt.Elem(), this way it would dereference 2 times and get to the concrete instance.

Thoughts?


Something like:

gt := reflect.TypeOf(i)

for gt.Kind() == reflect.Ptr {
	gt = gt.Elem()
}


ct, ok := cdc.typeInfos[gt]
if !ok {
return "", fmt.Errorf("no type registered for %s", gt.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a sentinel error, such that callers can switch over a returned error to see if the type wasn't registered or if any other error occurred.

And independent of that: I think %s already calls String() on the type (no need to explicitly call it here).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would those two errors be okay for the purpose?

  • ErrNilPointer, when i is nil;
  • ErrTypeNotRegistered, when the concrete type of i is not registered;

Yup, String() will be removed :-)

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 this pull request may close these issues.

2 participants