Possible bug in starport/pkg/cosmosclient/cosmosclient.go relating to address prefix
Describe the bug
There's a WithAddressPrefix option in cosmos client which allows the setting of an address prefix. This prefix is stored in client and applied in BroadcastTxWithProvision with the following code:
config := sdktypes.GetConfig()
config.SetBech32PrefixForAccount(c.addressPrefix, c.addressPrefix+"pub")
The problem is in the process of creation of client object, an AccountRegistry object is created (in New function) which contains a keyring which is initialized with the old prefix. If one tries to get address of any key the returned string would contain the old/default prefix. Moreover BroadcastTx will fail because the although the prefix is set before getting account address of the key, the keyring and AccountRegistry still use the old prefix, resulting in rpc error: code = InvalidArgument desc = invalid Bech32 prefix.
To Reproduce Steps to reproduce the behavior:
- Run a blockchain with an address prefix other than
cosmos. - Run a client similar to https://docs.starport.com/guide/blog/connect-blockchain.html but replace
cosmosclient.New(context.Background())withcosmosclient.New(context.Background(), cosmosclient.WithAddressPrefix("newPrefix"))
Please provide the version output
-
starport version: 0.19.3 (the related code is the same as the one in development branch)
How to fix
The user can circumvent this by calling SetBech32PrefixForAccount before the creation of client. In my opinion the more elegant way is for starport library to call it in New function, before creation of AccountRegistry:
c.AccountRegistry, err = cosmosaccount.New(
cosmosaccount.WithKeyringServiceName(c.keyringServiceName),
cosmosaccount.WithKeyringBackend(c.keyringBackend),
cosmosaccount.WithHome(c.homePath),
)
Ran into the same bug today, however, problem for me was the fetching of account.Address(accountName) that was coming back with cosmos prefix even after changing the hardcoded prefix in cosmosaccount.go and cosmosclient.go.
In my opinion there should be a more elegant way of changing prefixes in the client, maybe a config file that would fetch the prefix from one point.
I created a PR to add prefix change in client creation as well: https://github.com/ignite/cli/pull/2743 This could solve your issue.
In my opinion there should be a more elegant way of changing prefixes in the client, maybe a config file that would fetch the prefix from one point.
This is actually not elegant, but the SDK currently relies on this global configuration, so we must use it if we use SDK methods to send transaction. Possible we should ask for a refactoring in the SDK to remove or decrease the dependency to this global config and add more flexibility like the ability to provide a custom prefx manually when sending a transaction
@lubtd regarding my last comment of the mentioned PR, do you think we could create an issue in the SDK regarding the missing prefix in the cache key of the sdk.Address.String() ?
I ask because they may be disinterested in a fix that concerns a multi-prefix context, because they always use a single one.
@DiscreteLogarithm Can you confirm #2743 has fixed this bug ?
Closing this as stale, please reopen if otherwise.