-
Notifications
You must be signed in to change notification settings - Fork 352
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
Problem: current code is not based on stargate scaffold #55
Conversation
add-genesis-account not working. i ran this command) |
@leejw51crypto @yihuang it seems this re-generated code 1) doesn't set up bech32 prefixes, 2) does cro->basecro conversion? |
generation fine, but
|
copied the prefix module from master branch.
did the conversion by hacking the cli arguments. |
f1adb50
to
612cd86
Compare
cmd/chain-maind/cmd/root.go
Outdated
if coin.Denom == "cro" { | ||
coin.Denom = "basecro" | ||
coin.Amount = coin.Amount.Mul(sdk.NewInt(100000000)) | ||
} |
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 this instead call sdk.ConvertCoin(amount, app.BaseCoinUnit)
? just in case we have other units (e.g. "millicro")
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.
If we call sdk.ConvertCoin
, we need also register the denominations in cosmos-sdk, the alternative is we just maintain all the conversions here, since the chain itself only care about basecro.
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.
one needs register other types of things in SDK -- it'll be a bit easier to have that in one place and not duplicate what's provided by SDK
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.
is it merged?
in my testing, it's not done automatically
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.
it's possible to use "denom_metadata"
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.
is it merged?
in my testing, it's not done automatically
It seems they only added the ConvertCoin
utility, don't call it in cli. we calls it when pre-process command arguments.
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.
one needs register other types of things in SDK -- it'll be a bit easier to have that in one place and not duplicate what's provided by SDK
done
cmd/chain-maind/cmd/root.go
Outdated
func convertDenom(args []string) { | ||
if len(args) >= 5 && args[0] == "tx" && args[1] == "bank" && args[2] == "send" { | ||
args[5] = convertCoin(args[5]) | ||
} else if len(args) >= 4 && args[0] == "tx" && args[1] == "staking" && args[2] == "delegate" { |
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.
+ unbond + redelegate?
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.
add-genesis-account
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.
also in "init" -- it was modified to put "basecro" as unit in other modules (gov, staking, ...)
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.
added
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.
@yihuang for init, maybe changing "NewDefaultGenesisState" ?
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.
@yihuang for init, maybe changing "NewDefaultGenesisState" ?
it seems already working? I change the denom in config.yml
to "cro", it get translated to "basecro" in genesis.json
after starport --serve
.
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.
oh, you mean config of other modules, like staking's bond_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.
yes
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.
before it was doing this https://github.com/crypto-com/chain-main/blob/master/cmd/chain-maind/init.go#L112
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.
Supported the genesis patch by add post-processing to the init command, similar to how starport works.
Current code fails with following backtrace:
|
@yihuang the error I traced to is " parameter BondDenom not registered" |
fixed, seems caused by keeper initialization order. |
In my local test, both send tx and delegate tx works, I guess it's good to switch to reviewable now ;D |
tried locally now and it seems to work |
Solution: - re-generated with starport's stargate branch - ported the fixes (bech32 prefix, bip44 path, denomination conversion) to chain command line
/runsim |
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.
LGTM. Just some lint issues.
Solution: