-
Notifications
You must be signed in to change notification settings - Fork 221
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
Upgrade cosmos sdk to latest release. #1343
Conversation
I think we may have to backport all the bug fixes I made into the "latest" version of cosmos SDK again |
Akash: cosmos lib 0.43.0-rc2 upgrade
|
For tendermint, the only changes I made are the following
The changes have not been made in the tendermint version you are targeting and needs to be backported by forking that tag and applying them in our repo. |
For cosmos SDK, we made the following changes in our tag v0.41.4-akash-4
This change needs to be backported
This change has been partially applied in the new version. The changes to the functions
This change has been applied and does not need to be backported |
The tendermint fix we definitely want to carry forward if upstream hasn't merged it yet. |
ad25044
to
6f918fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #1343 +/- ##
==========================================
- Coverage 42.77% 42.48% -0.29%
==========================================
Files 257 272 +15
Lines 15192 15327 +135
==========================================
+ Hits 6498 6512 +14
- Misses 8001 8112 +111
- Partials 693 703 +10 |
We need to correctly set We need to write migration for address if we are storing the address in a module similar to this code. https://github.com/cosmos/cosmos-sdk/blob/0ab5726a7aa5c2d66916b14becb696e6e19a1efb/x/bank/legacy/v043/store.go#L51 I suspect Ref: |
@arijitAD yes, |
@arijitAD does new address format have prefix defining address length in it? |
From this migration, it seems that 1-byte prefix is allocated for length. https://github.com/cosmos/cosmos-sdk/blob/0ab5726a7aa5c2d66916b14becb696e6e19a1efb/x/bank/legacy/v043/store.go#L55 |
i see, then we have to set it same way by adding extra byte for address type |
Let's use type instead of length for the prefix if possible. Also, if we're going to go and rewrite our indexes, we should use the binary/packed address instead of the base 16 ascii representation representation. |
yah, sorry, i meant to say extra byte as address type but was also curious if new address has any sort of meta information in it to determine it's length. it's not a case anyways and extra byte for type is only option |
iter := sdk.KVStorePrefixIterator(store, certificatePrefix(owner)) | ||
defer func() { | ||
_ = iter.Close() | ||
}() |
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.
The iter
was declared but wasn't used. Seemed unnecessary, so removed.
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.
🙏
sdkutil/address.go
Outdated
// GetAccAddressFromBech32 creates an AccAddress from a Bech32 string. | ||
// It internally calls `sdk.AccAddressFromBech32` and ignores the error. | ||
func GetAccAddressFromBech32(address string) sdk.AccAddress { | ||
addr, _ := sdk.AccAddressFromBech32(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.
should we set here panic if address is invalid?
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 had done that initially, but, that causes TestCreateBidNonExistingOrder
to fail here: https://github.com/ovrclk/akash/blob/cosmos-upgrade/x/market/handler/handler_test.go#L134 because it passes an empty string for order owner and provider address.
I guess, we can set those strings to a valid address that doesn't exist in the store to make the test pass.
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.
Done
@@ -21,7 +21,7 @@ done | |||
# combine swagger files | |||
# uses nodejs package `swagger-combine`. | |||
# all the individual swagger files need to be configured in `config.json` for merging | |||
swagger-combine ./client/docs/config.json \ | |||
.cache/node_modules/.bin/swagger-combine ./client/docs/config.json \ |
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 should pick swagger-combine from PATH as we manually set it. doesn't it work?
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.
simply running make codegen
wasn't working locally for me without this change.
x/cert/keeper/key.go
Outdated
|
||
var ( | ||
prefixCertificateID = []byte{0x01} | ||
keyAddrPrefixLen = 1 /*len(PrefixCertificateID)*/ + 1 /*owner_address_len (1 byte)*/ |
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.
something like that to make it more readable?
keyAddrPrefixLen = 2 // 1 byte for PrefixCertificateID) followed by owner_address_len byte
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.
const
?
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 is const
. The diff shows var
on the LHS, it is already const
on the RHS.
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 great @arijitAD.
For the prefixes, I would really like to use functions that return new objects than to share mutable state if it is possible.
I believe that this will affect querying - possibly searching or desearializing indexes and data. Take a look at these queriers in the modules to be sure.
} | ||
|
||
endBz := oldStoreIter.Key()[V012Bech32AddrLen:] | ||
newStoreKey := append(append(prefixBz, address.MustLengthPrefix(addr)...), endBz...) |
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.
Interesting that they use just the length instead of a type (which could in also encode the length).
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.
Yeah! they call these variable-length addresses. So, using length is a generic way to encode and be future proof if some new address of length say 40 gets introduced. The max length they have set for an address is 255, so well within a byte.
x/cert/keeper/key.go
Outdated
|
||
var ( | ||
prefixCertificateID = []byte{0x01} | ||
keyAddrPrefixLen = 1 /*len(PrefixCertificateID)*/ + 1 /*owner_address_len (1 byte)*/ |
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.
const
?
app/app.go
Outdated
@@ -454,10 +452,46 @@ func NewApp( | |||
return app | |||
} | |||
|
|||
func (app *AkashApp) registerUpgradeHandlers() { | |||
app.keeper.upgrade.SetUpgradeHandler("v0.43", func(ctx sdk.Context, plan upgradetypes.Plan, |
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.
Please suggest a suitable name for the upgrade handler. Currently, it is named v0.43
which is based on the cosmos version we are upgrading to. Should it be something like: v0.13
as that will be the version of the next akash release?
Or should it be something like: akashnet-2-upgrade-1
as it was previously?
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 v0.13.0_v0.43 where first part is akash version and 2nd is cosmos
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.
Nice idea! Named it: akash_v0.13.0-cosmos_v0.43.0
.
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 do it using this pattern akash_v0.13.0_cosmos_v0.43.0
as -
is valid symbol in semantic versioning indicating it is prerelease
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.
Done
// there is nothing left over in the validator fee pool, so as to keep the | ||
// CanWithdrawInvariant invariant. | ||
// NOTE: staking module is required if HistoricalEntries param > 0 | ||
// NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC) |
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.
Did you find this information somewhere in the Cosmos docs or just figure it out from reading the code?
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.
Took it from cosmos sdk code
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 you put a link to that in the source code? Someone else may want to look at that as well.
sdkutil/address.go
Outdated
import sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
||
// MustAccAddressFromBech32 creates an AccAddress from a Bech32 string. | ||
// It internally calls `sdk.AccAddressFromBech32` and ignores the error. |
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 comment is wrong? It panics if there is an error.
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.
Thanks, corrected.
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 great! I might have missed it, but I didn't see address migration for x/deployment
?
func MigrateStore(ctx sdk.Context, storeKey sdk.StoreKey) error { | ||
store := ctx.KVStore(storeKey) | ||
v013.MigratePrefixBech32AddrBytes(store, types.DeploymentPrefix()) | ||
v013.MigratePrefixBech32AddrBytes(store, types.GroupPrefix()) |
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.
Do the addresses need to be migrated from ascii/hex to binary 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.
That is what is being done by v013.MigratePrefixBech32AddrBytes()
. It migrating addresses from the hex encoded bech32 string format to length prefixed binary format.
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.
🚀
bcbac60
to
5279545
Compare
Description
Motivation and Context
design/approved
labelHow Has This Been Tested?
Types of changes
Checklist:
git commit -s