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

Amino Audit Outline #8

Merged
merged 24 commits into from
Sep 13, 2020
Merged

Amino Audit Outline #8

merged 24 commits into from
Sep 13, 2020

Conversation

taariq
Copy link
Contributor

@taariq taariq commented Jul 23, 2020

AMINO Legacy REST endpoint audit.

Rendered:
https://github.com/cosmosdevs/stargate/blob/cosmos-amino-audit-api-endpoints/audit.md

To Do

  • Review from the SDK devs
  • Complete audit of transaction construction endpoints
  • Tendermint endpoints like Block

Copy link
Member

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Seems super helpful but maybe would be good to get it into the SDK repo as a clearer summary of some aspects of the changelog? Probably worth a document in the SDK itself that summarizes what the Stargate upgrade was all about, how things changed, and specifics like this no how folks need to upgrade. In Tendermint repo we've had UPGRADING.md which serves this purpose beyond the changelog record of what changed. Maybe this could be the start of an UPGRADING.md for the SDK?

audit.md Outdated Show resolved Hide resolved

The Legacy Amino interface provides an interface most similar to cosmoshub-2 and prior versions of the Cosmos SDK. This interface is available for both querying and sending transactions. It is implemented by executing Amino JSON serialization refliection over the new set of protobuf compatible structs.

The core finding of this audit is that while changes to the underlying structs result in an interface that is close to the prior version.
Copy link
Member

Choose a reason for hiding this comment

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

Unfinished sentence?

* **Endpoint Name:** QueryDelegatorDelegations
* **Endpoint Path:** ```"/staking/delegators/delegations"```
* **What Changed:**
* ```“balance”``` now is no longer a number. It is a field with two values: ```"amount"``` and ```"Denom”```
Copy link
Member

Choose a reason for hiding this comment

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

Should Denom be capitalized here? I believe JSON fields are all supposed to be lower case but there's been some inconsistency in the past

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That uppercase may have been a bug. I'll check on it.

@zmanian zmanian merged commit 06a04b5 into master Sep 13, 2020
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

3 participants