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

Max automatic token associations #81

Merged
merged 5 commits into from
Aug 19, 2021

Conversation

anighanta
Copy link
Contributor

@anighanta anighanta commented Aug 17, 2021

Description:

Related issue(s):

Fixes #80

Notes for reviewer:
Added a new field to specify the max automaic associations that an Account can be involved in

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Copy link
Contributor

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

LGTM

qnswirlds
qnswirlds previously approved these changes Aug 17, 2021
steven-sheehy
steven-sheehy previously approved these changes Aug 17, 2021
@@ -53,4 +53,5 @@ message CryptoUpdateTransactionBody {
google.protobuf.BoolValue receiverSigRequiredWrapper = 13; // If true, this account's key must sign any transaction depositing into this account (in addition to all withdrawals)
}
google.protobuf.StringValue memo = 14; // If set, the new memo to be associated with the account (UTF-8 encoding max 100 bytes)
google.protobuf.UInt32Value max_automatic_token_associations = 15; // The maximum number of tokens that an Account can be implicitly associated with. Up to a 1000 including implicit and explicit associations.

Choose a reason for hiding this comment

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

I'm confused by the second part of this comment. Is this saying the total max associations is 1000? I would just have it similar to how it is in CryptoCreate

ijungmann
ijungmann previously approved these changes Aug 17, 2021
Copy link

@ijungmann ijungmann left a comment

Choose a reason for hiding this comment

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

Have a question about a comment, but the code looks good.

Nana-EC
Nana-EC previously approved these changes Aug 17, 2021
Copy link
Contributor

@Nana-EC Nana-EC 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.
I'll have questions on the service implementation around what determines which tokens get applied to the implicit association (filtering) and what happens after the max is met in terms of extension / or removal

@steven-sheehy
Copy link
Member

Need to signoff your commits to pass DCO

@@ -465,6 +465,7 @@ message TokenRelationship {
TokenKycStatus kycStatus = 4; // The KYC status of the account (KycNotApplicable, Granted or Revoked). If the token does not have KYC key, KycNotApplicable is returned
TokenFreezeStatus freezeStatus = 5; // The Freeze status of the account (FreezeNotApplicable, Frozen or Unfrozen). If the token does not have Freeze key, FreezeNotApplicable is returned
uint32 decimals = 6; // Tokens divide into <tt>10<sup>decimals</sup></tt> pieces
bool automatic_association = 7; // Specifies if the relationship is created implicitly. False : explicitly associated, True : implicitly associated.
Copy link
Contributor

Choose a reason for hiding this comment

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

does the current state hold the explicit / implicit info, just not exposed?

xin-hedera
xin-hedera previously approved these changes Aug 18, 2021
@tinker-michaelj tinker-michaelj dismissed stale reviews from qnswirlds and themself via 95890ad August 19, 2021 22:00
@tinker-michaelj tinker-michaelj merged commit 3a02987 into develop Aug 19, 2021
@tinker-michaelj tinker-michaelj deleted the 079-D-MaxAutomaticTokenAssociations branch August 19, 2021 22:05
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.

8 participants