Skip to content
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

Cw20 state key compatibity with previous versions #346

Closed
MSNTCS opened this issue Jul 23, 2021 · 7 comments
Closed

Cw20 state key compatibity with previous versions #346

MSNTCS opened this issue Jul 23, 2021 · 7 comments

Comments

@MSNTCS
Copy link

MSNTCS commented Jul 23, 2021

Hello there,

In this CW20 version version = "0.2" we used byte string as a key for rhis state.

const TOKEN_INFO_KEY: &[u8] = b"token_info";

In the new version of CW20 version = "0.7.0", however, as we use storage_key.as_bytes() for Item, the key that we use here is not the identical as the previous key.

pub const TOKEN_INFO: Item<TokenInfo> = Item::new("token_info");

For migrating the contract to this new version, we have the problem of not having the same key.

 error: 'rpc error: code = Unknown desc = cw20_base::state::TokenInfo not found: contract query failed'

We must put the length bytes at the first for compatibility with the legacy singleton store.

Is this new key would be the new format for the TOKEN_INFO key or can we have it compatible with previous versions?

@ethanfrey
Copy link
Member

You are referring to cw20-base, not cw20, right?

In terms of "compatibility with legacy", I see no need to force our data structures to be the same. Rather, what is missing is a migrate function that reads the legacy version and writes the new version.

I would be happy to accept such a PR on the 0.7 branch, so it can update from 0.2

@MSNTCS
Copy link
Author

MSNTCS commented Jul 28, 2021

What I meant was cw20 base. We had other issues as well regarding the compatibility with the previous versions (Using Addr for balance and allowance instead of CanonicalAddr). I made this for the compatibility issues.

@ethanfrey
Copy link
Member

ethanfrey commented Jul 28, 2021

I don't see a good reason to make a new contract byte-for-byte compatible with an old contract.
That is what the migrate function is for. You can read the state, save it to the new format, and delete it.

This may take quite some gas (1 time), but unless you have 10000+ token holders, I think it is quite worth it.

(read, write, delete) = ~5000 gas per entry.
10,000 accounts = 50 million gas. A lot, but would fit in a block and probably cheaper than dev time maintaining a legacy version 😉

@MSNTCS
Copy link
Author

MSNTCS commented Jul 29, 2021

To be more specific, we would not be able to handle migration while migrating the code itself which is a specific case with terra migration from Columbus-4 to Columbus-5; that's why we had to publish a specific version of the cw20 base that is compatible with the previous version. We cannot have the migration handle to handle this situation. I would appreciate it if you could mention the problem that might happen in case we keep the compatibility with the previous versions.

@ethanfrey
Copy link
Member

It's not bad, it is more a lot of maintenance if you want to follow cw20 changes (but I guess there are not too many).

I see your situation is unique. What makes sense to me is:

  1. Make this byte-for-byte compatible contract like you did.
  2. Swap out the new 0.16-compatible contract in the Terra upgrade
  3. Provide a migrate function on cw20-base that ports from the old layout (from your legacy contract)
  4. Once columbus-5 is live, people can choose to migrate their "legacy cw20" to the modern "cw20". This probably doesn't make sense until a new feature is added to cw20, but it does provide a path and doesn't leave you with the burden of back porting to "legacy cw20" indefinitely

@MSNTCS
Copy link
Author

MSNTCS commented Jul 30, 2021

Thank you for the reply and your comments on this issue. We will follow this path to get back to the current version of cw20 to avoid future discrepancies.

@ethanfrey
Copy link
Member

I think you guys are handling this and no work is needed in the core lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants