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

Upgrade to telescope #40

Closed
wants to merge 29 commits into from
Closed

Conversation

pyramation
Copy link
Contributor

No description provided.

@pyramation
Copy link
Contributor Author

pyramation commented Oct 3, 2022

@webmaster128 the only potential blocker I see is the eslint disable, do we need this?

telescope already has a ts lint disable mechanism, so we can add one for eslint if needed.

@webmaster128
Copy link
Member

@webmaster128 the only potential blocker I see is the eslint disable, do we need this?

telescope already has a ts lint disable mechanism, so we can add one for eslint if needed.

For me this is nice to have. I don't really need it. However when people copy/paste generated files around it is convenient for them that the linting is disabled by default without them having to configure something in their project setup.

uninterpretedOption: UninterpretedOption[];
}
export interface MethodOptions {
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

order of these changed, but it doesn't change the behavior. This is due to the many nested sub types. Looks good in terms of structure.

ApplySnapshotChunk(request: RequestApplySnapshotChunk): Promise<ResponseApplySnapshotChunk>;
}

export class ABCIApplicationClientImpl implements ABCIApplication {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this used?

Copy link
Contributor Author

@pyramation pyramation left a comment

Choose a reason for hiding this comment

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

A few minor things, anything more than minor I don't think actually hits any codepaths.

Copy link
Contributor Author

@pyramation pyramation left a comment

Choose a reason for hiding this comment

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

A few minor things, anything more than minor I don't think actually hits any codepaths.

/** grpc-gateway_out does not support Go style CodID */
/**
* grpc-gateway_out does not support Go style CodID
* pagination defines an optional pagination for the request.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this comment seems to be the only one due to a newline that is stuck. I suggest we fix the proto for this one.

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.

So, finnally got the chance to look into this. I found a few more things, but overall this is very cool.

package.json Outdated Show resolved Hide resolved
src/confio/proofs.ts Outdated Show resolved Hide resolved
src/confio/proofs.ts Show resolved Hide resolved
src/cosmos/authz/v1beta1/query.ts Show resolved Hide resolved
src/cosmos/authz/v1beta1/query.ts Outdated Show resolved Hide resolved
src/cosmos/authz/v1beta1/tx.ts Show resolved Hide resolved
Comment on lines +95 to +96
header_1: Header;
header_2: Header;
Copy link
Member

Choose a reason for hiding this comment

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

This would break the client code using the fields. Why are _ used here? All other fields are in camelCase, so the _ feels a bit misplaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@webmaster128 the protos look like this https://github.com/cosmos/ibc-go/blob/c8e2ebcf2eaa6b487ca52499063c7c3db4253398/proto/ibc/lightclients/tendermint/v1/tendermint.proto#L84-L85

message Misbehaviour {
  option (gogoproto.goproto_getters) = false;

  // ClientID is deprecated
  string client_id = 1 [deprecated = true, (gogoproto.moretags) = "yaml:\"client_id\""];
  Header header_1  = 2 [(gogoproto.customname) = "Header1", (gogoproto.moretags) = "yaml:\"header_1\""];
  Header header_2  = 3 [(gogoproto.customname) = "Header2", (gogoproto.moretags) = "yaml:\"header_2\""];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One solution that seems that it may work (at least for this case) is to camelCase the gogoproto.customname to get the expected result. Other than that, removing the underscore I don't think is a default to the camelCase function that is used by protobuf.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

specifically, the camelCase as defined in protobuf.js is this one (which does not remove the underscore)

var camelCaseRe = /_([a-z])/g;
const camelCase = function camelCase(str) {
    return str.substring(0, 1)
         + str.substring(1)
               .replace(camelCaseRe, function($0, $1) { return $1.toUpperCase(); });
}

So another approach could be to manually look for a pattern. We could use a pattern like /[a-zA-Z0-9]+_[0-9]+$/, we could remove the underscore?

  • use pattern?
  • camelCase gogoproto.customname
  • something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPDATE: I decided to use the assumption that if there is an underscore with only a number after it, then the underscore should be removed. This is only in the case where it ends with numbers only, if there are any words we leave it alone.

To be honest, I'm not super confident this change would break client code. If it did, it would break amino, not proto, and I'm actually wondering if this new change will break the client code (for amino) as the original developer wrote the prop names with underscores, and from what I've discovered the original authored casing is what typically matters in amino.

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@pyramation
Copy link
Contributor Author

I will dig into the optional fields today to get your questions answered, but also fix it so they match the ts-proto output. Thanks for the useful info.

I think most should be solvable. I'll be tracking the issues here cosmology-tech/telescope#237

@pyramation
Copy link
Contributor Author

kept comments here but to avoid confusion with older changes, I've superseded this PR with this one: #44

@pyramation pyramation closed this Oct 12, 2022
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.

None yet

2 participants