Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial draft of PBJ types for token state #263
Initial draft of PBJ types for token state #263
Changes from 11 commits
ba1af89
214df5f
b39dec9
f624257
5f39ec7
a5db1cc
a718906
3bd2503
f7ac736
6a8cb7e
435cea9
ffb79b3
bc3564e
1a71497
db3e917
04d9470
f2158c3
36ceb52
b65ba3b
1ebc222
8f54afe
1939032
a871898
1236702
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker for this PR, but we really should have better docs here. These schemas will generate Java objects that we will use in our code. So if I'm working on some code and I want to understand the Account class, I'm going to be looking at and reading that javadoc. So It would be nice to describe more what an account is, how it fits into our system, etc. At least at a high level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should describe a little better as to when having a key is optional and when it is not. Most accounts require it. Only special accounts like 0.0.800 permit an empty key. Maybe we should document that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And hollow accounts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all look good. However there is one point worth making, which is that in protobuf serialization, false is represented by omitting the data from the data, whereas "true" is represented by a single byte. So wherever we can, we should make sure the "false" setting is more frequent than the "true". In these three cases, that is what we see. most accounts have
deleted
as false, most accounts are not smart contracts, and most accounts don't havereceiver_sig_required
set to true, so for most accounts these three fields will encode as 0 bytes. Which is great.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great insight. Thanks !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason we use
int64
here, butint32
in associations? Should we useint32
in all these places? I'm not sure if anybody is going to have more than 2 billion NFTs, but I guess it could happen... but maybe they should use multiple accounts at that point?At the protobuf level int64 and int32 are just as efficient up until the limit of int32. But at the Java level it is a long vs. an int. Otherwise it wouldn't matter that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just used int64 where we were using long and
int32
forint
. Will revisit before these going to production.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if number_associations is maxed out at the int32 max value, but used_auto_associations are not maxed out, and somebody tries to then create a new auto_association? Do we error out, or do we end up overflowing on number_associations? Just queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what do we use number_associations for? Is it just for a response to a query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked with @tinker-michaelj . It is not only needed for query, but also for calculating fee for renewal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only needed to answer some queries? Or do we use it during checking for solvency? Or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed because account can't be deleted if there are any positive balances in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we end up with shard and realm encoded in these files, we'll need it here as well. Otherwise we can leave it off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the behavior if this is ever 0? Do we just make sure it never is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we are checking if auto renew seconds is in the range in precheck based on these properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be the only field that is smart contract specific (other than the smart_contract boolean). I'm wondering why we need it? Is it again for the purposes of a query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is needed for Storage rent and also for checking if number of KV pairs did not exceed the limit allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure these are right. It seems like we should be using our "map of linked list nodes" concept here instead. It isn't a blocker for this PR, but we need to discuss this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in the first 16 fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all keys should also be in first 16 fields , so the fields are already filled in. 🙂