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

[sui-framework] Added swap and swap_remove function to the table_vec.move #13644

Merged
merged 10 commits into from
Sep 16, 2023

Conversation

WayneAl
Copy link
Contributor

@WayneAl WayneAl commented Sep 6, 2023

Description

Added swap_remove function to table_vec.move since we have a situation to remove an certain element in the TableVec and both table and vector have similar functionality implemented.

Test Plan

New unit tests for sui::table_vec.


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Introduces Protocol Version 25, which adds sui::table_vec::swap and sui::table_vec::swap_remove to system packages. Use these functions to swap two positions in a single TableVec<T> or swap an element to the end and remove it in O(1) time, analogous to std::vector::swap and std::vector::swap_remove.

@vercel
Copy link

vercel bot commented Sep 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 16, 2023 4:39pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 16, 2023 4:39pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2023 4:39pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2023 4:39pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Sep 16, 2023 4:39pm

@WayneAl
Copy link
Contributor Author

WayneAl commented Sep 7, 2023

After testing, I found the remove function may cause the empty dynamic field.

So, I implemented the other function swap.
We can swap the ith element with last, then pop_back for the purpose of removing the ith element.

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

I think you're missing the remove function here but swap looks mostly there! Can you also add some tests for these new functions?

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

+1 to @amnn's comment. And could you add tests in table_vec_tests.move? Thanks!

@WayneAl
Copy link
Contributor Author

WayneAl commented Sep 14, 2023

  1. if (i == j) { return }; added
  2. tests in table_vec_tests.move added

There is no remove function because I think it can be done with swap with i th and last, then pop_back.

For example, if I want to remove i th element from TableVec v:

table_vec::swap(&mut v, i, table_vec::length(&v) - 1 );
table_vec::pop_back(&mut v)

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Thanks @WayneAl, could you add a couple more tests? I've noted them in the inline comments.

Also, regarding remove, we actually have an API called swap_remove on vector:

public fun swap_remove<Element>(v: &mut vector<Element>, i: u64): Element {

Which is analogous to what you're trying to do here, so it wouldn't be a bad idea to include that, but that's not technically a blocker for this PR (as long as we rename it to mention swap rather than remove).

Copy link
Contributor

Choose a reason for hiding this comment

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

Some more test cases to add here:

  • i == j (succeeds)
  • i == j > length (aborts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added !
And I also added the swap_remove and its test.

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @WayneAl ! The PR needs some tests fixed, leave it with me, and I'll prepare and land it over the next couple of days.

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.

None yet

3 participants