Add GasToken extension GetAccounts for Governance token#4424
Add GasToken extension GetAccounts for Governance token#4424cschuchardt88 wants to merge 4 commits intoneo-project:masterfrom
GasToken extension GetAccounts for Governance token#4424Conversation
erikzhang
left a comment
There was a problem hiding this comment.
First, this implementation is unreasonable. If a GetAccounts method is needed, it should be added to TokenManagement. Secondly, returning all accounts at once is a very expensive operation, considering that the number of accounts could be very large; an iterator should be used instead. Therefore, I'm not sure why this method was added in the first place.
| if (!snapshot.Contains(key)) | ||
| throw new InvalidOperationException("The asset id does not exist."); | ||
| key = CreateStorageKey(Prefix_AccountState, account, assetId); | ||
| key = CreateStorageKey(Prefix_AccountState, assetId, account); |
There was a problem hiding this comment.
Putting "account" first is a more logical approach. This is because finding all assets in a wallet is a common operation, while finding all accounts for a specific token is a very rare operation.
There was a problem hiding this comment.
If its done this way than you can't lookup the storage key without knowing an account that has a balance of the asset. So without the account you wont find any assets for a given token. This is basic relational data/database techniques and standard data/database normalization
|
Think about it.... Keep in mind |
|
We need to return |
|
Also, we have two options to achieve this:
|
|
After some further investigation, I found that From my understanding, these two methods are primarily intended for use by block explorers. However, if a block explorer aims to support explicitly listing all accounts holding an arbitrary asset, these two methods alone are insufficient. The reason is that other NEP-17 assets do not provide a corresponding As a result, a block explorer still has to scan all NEP-17 transfer notifications in order to reconstruct the complete set of accounts. Under this assumption, the existence of |
vncoelho
left a comment
There was a problem hiding this comment.
Why not giving RestServer Plugin read-only access to do it the way it wants? Thus, avoiding implementing it here.
Description
Note: Not Having breaks all
neo-noderepo andRestServerplugin.GasTokenExtensions.csfile was deleted. This PR adds back the the extensions the renamedGasTokenasGovernancetoken. Also I switch the placement of the storage key forTokenManagement's account state prefix for easier and fast storage lookups for the token asset accounts.Change Log
GovernanceExtensions.csfileType of change
How Has This Been Tested?
Checklist: