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

fix: treat addresses as strings in sdk internals #298

Merged
merged 10 commits into from
Sep 3, 2021
Merged

fix: treat addresses as strings in sdk internals #298

merged 10 commits into from
Sep 3, 2021

Conversation

egonspace
Copy link

@egonspace egonspace commented Jul 22, 2021

Description

We had a chronic problem with the high cost of marshal/unmarshal of Account, Balance. This is due to the address bytes <-> bech32 address type conversion.

Of course, we've alleviated this problem to some extent by caching the pairs of bech32 strings and bytes, but it's more preferable that this happens so frequently that it's essentially unnecessary than cache. In other words, this cost is not required if the address is stored as the string type. It is only necessary to convert string address of an incoming msg to bytes when validating. Alternatively, if we only validate by checking bech32 prefix and checksum without converting to bytes, we will be able to do so at a much lower cost. (This could be carried out in a separate PR later.)

Main changes:

  • Addresses are all stored in string type in the store. A once-stored address does not need to be validated later. No bech32 encoding/decoding cost when we marshal/unmarshal it.
  • Cannot typecast address bytes with AccAddress directly. -> We must convert using BytesToAcAddress() function.
  • Unable to directly compare an AccAddress value and a ValAddress value. -> One AccAddressToBytes()ed must be compared to a ValAddress value that is a result of BytesToValAddress
  • Ostracon still records validator address as byte.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@egonspace egonspace self-assigned this Jul 22, 2021
@egonspace egonspace added this to the performance improvement milestone Jul 22, 2021
@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@7214594). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #298   +/-   ##
=======================================
  Coverage        ?   53.21%           
=======================================
  Files           ?      642           
  Lines           ?    47408           
  Branches        ?        0           
=======================================
  Hits            ?    25226           
  Misses          ?    19322           
  Partials        ?     2860           

client/keys/show_test.go Show resolved Hide resolved
Makefile Show resolved Hide resolved
crypto/keyring/output.go Outdated Show resolved Hide resolved
types/address.go Show resolved Hide resolved
types/address.go Show resolved Hide resolved
types/address.go Show resolved Hide resolved
x/auth/client/cli/query.go Show resolved Hide resolved
x/auth/keeper/grpc_query.go Show resolved Hide resolved
x/crisis/keeper/msg_server.go Outdated Show resolved Hide resolved
@egonspace egonspace mentioned this pull request Sep 3, 2021
4 tasks
Copy link
Contributor

@whylee259 whylee259 left a comment

Choose a reason for hiding this comment

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

LGTM, The address validation will be discussed in another PR.

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.

3 participants