-
Notifications
You must be signed in to change notification settings - Fork 375
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
feat(gnovm/tm2): add ZeroAddress constant #2401
base: master
Are you sure you want to change the base?
feat(gnovm/tm2): add ZeroAddress constant #2401
Conversation
@leohhhn |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2401 +/- ##
==========================================
- Coverage 60.44% 60.43% -0.01%
==========================================
Files 563 563
Lines 75159 75159
==========================================
- Hits 45431 45424 -7
- Misses 26341 26345 +4
- Partials 3387 3390 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ebe8540
to
5911266
Compare
Additional, I am not sure that init a ZeroAccount with account_number=0 when init chain makes sense. I made this for readability and clearly view point from user. |
@@ -91,4 +91,10 @@ func (bnk *SDKBanker) RemoveCoin(b32addr crypto.Bech32Address, denom string, amo | |||
if err != nil { | |||
panic(err) | |||
} | |||
// TODO: once TotalCoin is implemented, we need to deduct the TotalCoins beside send coin to ZeroAddress (?) | |||
zeroAddr := std.ZeroAddress() | |||
_, err = bnk.vmk.bank.AddCoins(bnk.ctx, zeroAddr, std.Coins{std.Coin{denom, amount}}) |
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 adding the removed coins to the zero address?
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.
@moul
This is because of the burn process definition: Should send
coins from burner to zero address, beside removing that amount of coins from Total Supply.
Adding to ZeroAddress making everyone can see how many coins is burned, and how many coins zero address currently holds. Deducting coins from total supply is the target of burning theory.
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 about having this constant defined in a p/
package instead? this way we don't need to manage multichain bech32 support.
Is it good to have a |
Just noticed that the |
Ah yeah, agree with this |
d5e1b54
to
a5df32c
Compare
I turn this PR to |
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the descriptionAfter discussion in #2316, this PR does:
std.ZeroAddress()
with addressg100000000000000000000000000000000dnmcnx
in tm2.zeroAddress
when user burn coin withSDKBanker.RemoveCoin()
ZeroAccount
withaccount_number
= 0 when initializing the chainstd.ZeroAddress()
ingnovm/std
in oder to help realm can burn coins to this address.There are 2 main usecase of this adding that I think about:
ZeroAddress
grc20
, user can burn coin by calling transfer toZeroAddress
:Status:
In this approach, I think we should deduct the total supply coin / token in chain, but it is neither implemented in native coin, nor
grc20
standard. This may be relate to this @leohhhn 's PR #2314.Demo:
Native coin
grc20 token - I changed admin address to mine when testing
This is my first implementation about zero address in gno ecosystem. I really want to know if there is any idea to make this better!