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
chore: added new field
account-id
with uint64 inAccountAddressByID
#13780chore: added new field
account-id
with uint64 inAccountAddressByID
#13780Changes from 9 commits
82ef03d
bd53cb1
335db34
4b944b8
ce167ea
94d4cf2
2289441
7f468bb
a272769
dd4d69f
98b9113
7e09506
3bc2c5e
9592596
21d8396
8a622ea
6302d29
2cf1d2c
5875b96
bdbaaa2
c270cec
03b5bd0
2b32933
037b354
78d46b9
34130b5
93b888e
c3b198c
1d680e7
718c752
faee1f8
3a39765
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
TBH I'm not sure how much value this case adds. IMO we should instead enforce one or the other value is passed. If both are passed, i.e. both are non-zero, then simply error.
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.
Unforunately we can't do this since
0
is also a valid case here to query byaccount-number
. and the default values for bothint64
&uint64
are0
sThere 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 start account numbers at 1?
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, but I'm getting the response for
0
too by using the same 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.
Yeah I see that they start with
0
. It' really unfortunate we have to have this sort of case. I would almost prefer to completely omit/ignore the legacy field all together.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 also agree with ignoring the legacy field. Maybe we can throw an error if it's not zero? So that users have immediate feedback they need to change something on their side.
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 are keeping it as deprecated means, may be it should not throw error unitl the field gets removed right? (not sure just a thought)
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.
The question really is:
Are we backporting this to < 0.47.x? If not, then just ignore the legacy field and remove this case all together. If we are backporting to 0.46.x and 0.45.x, then we need to handle the use of the legacy field and I suppose this is fine.
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 vote no backport.
Edit: but again, I also don't like to ignore that old field. Imagina a dapp pinging that endpoint, and suddently (after node upgrade) it returns a different response. I propose to error if the legacy field is populated.
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.
The problem is a zero/default value
0
is technically valid (the very first account). So there is no notion of "set" vs "unset" unfortunately. Thats why I suggested ignoring it.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.