-
Notifications
You must be signed in to change notification settings - Fork 405
feat: r/coins
balance checker
#3899
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: moul <[email protected]>
🛠 PR Checks Summary🔴 Must not contain the "don't merge" label Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
If you add TotalSupply in this PR, just linking this PR as well for the docs: #2661 |
can we stop using the term "airdrop" for this feature (PR title, and a unit test having the term in the title); this PR isn't an airdrop checker, but a balance checker. |
) | ||
|
||
// TotalSupply returns the total supply of the specified denomination | ||
func TotalSupply(banker std.Banker, denom string) int64 { |
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.
can we have this API instead, and always use a readonly banker?
func TotalSupply(banker std.Banker, denom string) int64 { | |
func TotalSupply(denom string) int64 { |
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.
Modified it so that the banker object is created inside the function. I also considered placing the banker globally, but it seemed better to restrict the object's lifetime to within the function (It's also purer than global).
695412e
} | ||
|
||
func renderHomepage() string { | ||
return `# Gno.land Coins |
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.
homepage should give some information, and provide default links so that we can try the features with just a few clicks.
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 is currently a placeholder. It is must be updated later.
## Total Supply | ||
` + std.NewCoin(denom, totalSupply).String() + ` | ||
|
||
[Back to Home](/r/gnoland/coins:) |
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.
back is already available in the navigation breadcrumb.
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.
removed 695412e
[Back to Coin Info](/r/gnoland/coins:` + denom + `) | ||
[Back to Home](/r/gnoland/coins:) |
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.
remove this, we should use the navigation breadcrumb for 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.
} | ||
|
||
for _, tt := range tests { | ||
banker := std.NewBanker(std.BankerTypeRealmIssue) |
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.
why do we need to initialize a banker from this realm?
(if there is a good reason, why not a readonly one?)
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.
In std.BankerTypeReadonly
mode, issuing coins are blocked, so I use std.BankerTypeRealmIssue
mode only when setting up the test environment.
I think there won't be any issues because the inside of the TotalSupply
or AddressBalance
functions are set to read-only (modified to create a banker internally). please correct me, if I wrong.
gno.land/pkg/sdk/vm/builtins.go
Outdated
for _, acc := range accounts { | ||
if acc == nil { | ||
continue |
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 scalable. We need to find a more efficient solution, or we should refrain from offering this feature for now.
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 modified this function to process in a single loop. This is what I could think of for now. What do you think?
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 needs a change in the underlying banker module, so that it keeps track of the available tokens, rather than iterating on the accounts here.
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.
check comments.
banker.TotalSupply
and r/coins
balance checkerr/coins
balance checker
@notJoon, this is still not good. Please do not modify the tm2 code at all in this PR; just examples/. |
@moul If possible, could you clarify the implementation direction for the If we're not using banker, using grc20's token-related functions seems like the most possible alternative option. I should have confirmed this before starting, but it seems something went wrong from the beginning. I would appreciate your confirmation on this. Thank you. |
r/coins needs to display balances (using banker). Most of this PR code focuses on total supply, which is more complex and not urgently needed (we'll address that later). For now, r/coins should concentrate on displaying the user balance for a specific coin. There should be a .gno file for the realm that includes only a Render function, along with a few helpers and some unit tests. Initial specifications can be found at https://github.com/gnolang/gno/pull/3826/files. Please ignore the total supply, as it is not yet supported. |
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.
Looks good; we can make the rendering better in the next iteration.
router.HandleFunc("{denom}", func(res *mux.ResponseWriter, req *mux.Request) { | ||
// denom := req.GetVar("denom") | ||
// banker := std.NewBanker(std.BankerTypeReadonly) | ||
// res.Write(renderAddressBalance(banker, denom, denom)) | ||
panic("not supported yet") | ||
}) |
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 would error and make the user experience a bit worse; maybe a better option is to simply write out "page coming soon" or similar.
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.
func formatBalance(coin std.Coin) string { | ||
return "Balance: " + strconv.Itoa(int(coin.Amount)) + " " + coin.Denom | ||
} |
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.
Nitpick: you can use std.Coin.String()
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 can also reduce function calls and package usage. That's a very good suggestion.
16ed599
Co-authored-by: Leon Hudak <[email protected]>
Co-authored-by: Leon Hudak <[email protected]>
Description
Implemented the token inquiry function. Currently, only the feature to check coins at a specific address is provided, and the function to check the total quantity is not yet supported.
Example Usage:
/r/gnoland/coins:ugnot
- shows the total supply of ugnot/r/gnoland/coins:ugnot/g1...
- shows the ugnot balance of a specific addressRelated
TotalCoin
reference #2661