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

Add MIGRATING.md #591

Merged
merged 8 commits into from
Dec 22, 2021
Merged

Add MIGRATING.md #591

merged 8 commits into from
Dec 22, 2021

Conversation

maurolacy
Copy link
Contributor

Add breaking changes list / migration points for 0.10.x to 0.11.0.

Closes #583.

@maurolacy maurolacy self-assigned this Dec 21, 2021
Add breaking changes list / migration points for 0.10.x to 0.11.0
@maurolacy maurolacy force-pushed the 583-add-migrating.md branch from 2b1bb2d to 466b1c8 Compare December 21, 2021 16:54
@maurolacy maurolacy marked this pull request as ready for review December 22, 2021 09:10
@maurolacy
Copy link
Contributor Author

maurolacy commented Dec 22, 2021

Last issue needed for 0.11 release.

MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@maurolacy
Copy link
Contributor Author

maurolacy commented Dec 22, 2021

Cool. Merging this, and cutting 0.11 next. (we need #590 / #588 first)

@maurolacy maurolacy merged commit 5e4561f into main Dec 22, 2021
@maurolacy maurolacy deleted the 583-add-migrating.md branch December 22, 2021 10:02
// New map
signed_int_map_new: Map<i8, String> = Map::new("signed_int_map-v2");

signed_int_map
Copy link
Member

@ethanfrey ethanfrey Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dangerous (and I am rather sure doesn't compile).
You are writing to the same space you are reading from in an iterator.

let current = signed_int_map.range_raw(deps.storage, None, None, Order::Ascending).collect::<StdResult<Vec<_>>()?;

for (k, _) in current.iter() {
  // ?? how to delete... maybe IntKey8 uses the old format? maybe we can support OldIntKey8 that does it... would make it much easier
}

for (k, v) in current.iter() {
  let signed = i8::from_be_bytes(k);
  signed_int_map_new.save(deps.storage, signed, v)
}

This is very important issue, it would be good to have a test case for this. (Can be in 0.11.1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. Yes, I wrote that more or less as pseudo-code from memory. Will work on a proper test case / working migration example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note this only affects signed int keys, which are rather rare.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, all the other breaking changes, including the one about removing the primary key from the multi index key spec, are non-breaking in terms of storage / data format, which is great.

Copy link
Contributor Author

@maurolacy maurolacy Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are writing to the same space you are reading from in an iterator.

Not sure this is what you mean, but notice both maps in the example have different namespace / prefix. It is assumed the new contract code will rely on the new namespace after migration.

Copy link
Contributor Author

@maurolacy maurolacy Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  // ?? how to delete... maybe IntKey8 uses the old format? maybe we can support OldIntKey8 that does it... would make it much easier

Good point, I hadn't thought about that. IntKey supports the new format as well. Maybe we can change it to support the old format. Or introduce the OldIntKey wrappers for it.

The way to do it now would be

k[0] ^= 0x80;
let signed = i8::from_be_bytes(k);
signed_int_map.remove(deps.storage, I8Key::from(signed);

I think. It would be good to confirm / test this as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are writing to the same space you are reading from in an iterator.
Not sure this is what you mean, but notice both maps in the example have different namespace / prefix. It is assumed the new contract code will rely on the new namespace after migration.

Ah, that is a very good point/design.

In any case, one needs &Storage the other &mut Storage, and you cannot hold them both at the same time. (Thank you Rust)

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

Successfully merging this pull request may close these issues.

Add MIGRATING.md
3 participants