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

feat: allow to specify an AccountParser inside StargateClient #1102

Merged
merged 5 commits into from
Mar 30, 2022
Merged

feat: allow to specify an AccountParser inside StargateClient #1102

merged 5 commits into from
Mar 30, 2022

Conversation

RiccardoM
Copy link
Contributor

This PR introduces a new StargateClientOptions interface that allows setting custom options for StargateClient.

The most important option is the accountParser option, which must be an AccountParser instance and allows to provide the StargateClient a custom way of decoding Cosmos accounts. This is particularly useful to solve problems when reading account from chains that support custom account types (eg. the Desmos chain). This can solve problems like the one REStake is having: eco-stake/restake#334

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Good start.

Should we make accountFromAny public (maybe under different name) such that the parser can be implemented as custom type or default to accountFromAny implementation.

packages/stargate/src/signingstargateclient.ts Outdated Show resolved Hide resolved
packages/stargate/src/stargateclient.ts Outdated Show resolved Hide resolved
packages/stargate/src/stargateclient.ts Outdated Show resolved Hide resolved
@RiccardoM
Copy link
Contributor Author

accountFromAny

Do you have an example of how you would implement this?

@webmaster128
Copy link
Member

webmaster128 commented Mar 30, 2022

Do you have an example of how you would implement this?

Let's get in the user's perspective, imagine someone wants to create a client that supports /desmos.profiles.v1beta1.Profile. An implamentation then looks like this:

import { Profile } from "@desmos/generated/types/accounts";

function myParser(input: Any): Account {
  const { typeUrl, value } = input;

  if (typeUrl === "/desmos.profiles.v1beta1.Profile") {
      // version 1
      const baseAccount = Profile.decode(value)?..baseAccount;
      assert(baseAccount);
      return accountFromBaseAccount(baseAccount); // accountFromBaseAccount is not public

     // version 2
    return {
      address: address,
      pubkey: pubkey,
      accountNumber: uint64FromProto(accountNumber).toNumber(), // uint64FromProto is not public
      sequence: uint64FromProto(sequence).toNumber(),
    }
  } else {
     throw new Error("Found unsupported account type"); // is this desired? Do you want a fallback to standard Cosmos SDK accounts?
  }
}

So basically it's unclear to me if it is currently possible to write a good implementation of the new interface.

@RiccardoM
Copy link
Contributor Author

So basically it's unclear to me if it is currently possible to write a good implementation of the new interface.

I've actually implemented like the following:

export function profileFromAny(input: Any): Account {
  const { typeUrl, value } = input;
  switch (typeUrl) {
    case "/desmos.profiles.v1beta1.Profile": {
      const baseAccount = Profile.decode(value)?.account;
      assert(baseAccount);
      return accountFromAny(baseAccount);
    }

    default:
      return accountFromAny(input);
  }
}

This kind of implementation relies on the following things:

  • inside Go, there is no such thing as inheritance: everything is done thought composition
  • to correctly implement the Cosmos SDK AccountI interface for a new type, you have to wrap a generic AccountI instance into a new type (Profile in our case)
  • AccountI instances are encoded as Any objects

So, each custom AccountI implementation (like Profile) will end up having an account field that is an Any. In this case, we can simply use accountFromAny to get the proper Account instance

@webmaster128
Copy link
Member

I've actually implemented like the following:

Looks good. I missed that accountFromAny is public already. Using accountFromBaseAccount directly would be better, but if we don't need to make it public, this is also good.

In this case I consider this feature-complete. Can you add a CHANGELOG entry? Then 🐎🐎🐎

@RiccardoM
Copy link
Contributor Author

In this case I consider this feature-complete. Can you add a CHANGELOG entry? Then racehorseracehorseracehorse

Added, let me know if it can be improved

Copy link
Member

@webmaster128 webmaster128 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. Just the exports left

packages/stargate/src/accounts.ts Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
packages/stargate/src/stargateclient.ts Show resolved Hide resolved
@webmaster128 webmaster128 merged commit 13ce43c into cosmos:main Mar 30, 2022
@webmaster128
Copy link
Member

Released as part of 0.28.1

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