-
Notifications
You must be signed in to change notification settings - Fork 152
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
✨ New state querying scheme #3570
✨ New state querying scheme #3570
Conversation
1d19665
to
4ab9e78
Compare
dd94b60
to
e9e8b05
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.9-maintenance #3570 +/- ##
===================================================
+ Coverage 76.62% 79.15% +2.53%
===================================================
Files 339 339
Lines 11842 11842
===================================================
+ Hits 9074 9374 +300
+ Misses 2768 2468 -300 |
Adds more modular and reusable
AccountStateType
to GraphQL queries.As seen in its implementation, this is pretty lousy. I tried to remove
Currency
/FungibleAssetValue
context from state query, but this has resulted in severe API inconsistency. 😥This issue is mainly due to
IAccount
having two roles:null
to zero-amountFungibleAssetValue
for givenCurrency
.As it stands, there is no possible way to know (although whether this is necessary might be debatable) how
Currency
related data is stored within the storage entirely from the library side (libplanet) or the client side (lib9c).FungibleAssetValue
from anAddress
and aCurrency
pair, one cannot actually know whether the underlying data is0
ornull
. Considering how it is treated,Currency
is inherently a client-side provided context. This itself isn't much of a problem, if we think ofIAccount
as an interface to obfuscate the underlying data (or logic).Currency
's raw value (inIValue
) without the information ofCurrency
being provided in its entirety, even though what is only needed isCurrency.Hash
.Further examples of API inconsistency among across different types of queries are:
GetState()
is getting a singleIValue
from an address, we only havestates
and is missingstate
. Furthermore, anIValue
is returned implicitly as base64 format.Currency
related queries require an actual instance ofCurrency
as stated above, and returns an interpretedFungibleAssetValue
instance. Again, as stated above, there is no way to actually know its raw value (Integer
).ValidatorSet
, again, it is returned as aValidatorSetType
instance.The purpose of this new
AccountStateType
is to provide a way to query raw underlying data in a consistent manner reflecting the underlying data structure so that the interpretation can be done at another layer by post processing the result.Seems like maintaining cost of improper mock/fixture has surpassed creating a proper one from scratch. If it wasn't for pending 4.0 release, I would've created a new one, but had to scrap it. 😕