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

Added Stack storage. #786

Closed
wants to merge 12 commits into from
Closed

Conversation

itsHaseebSaeed
Copy link

@itsHaseebSaeed itsHaseebSaeed commented Aug 23, 2022

As discussed here. Added stack implementation to storage-plus

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2022

CLA assistant check
All committers have signed the CLA.

@itsHaseebSaeed itsHaseebSaeed changed the title Added Stack Added Stack storage. Aug 23, 2022
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Okay, good start, thanks. I think it's still a bit too complicated becasue the type has too many features. Ideally it just does this: https://en.wikipedia.org/wiki/Stack_(abstract_data_type)#/media/File:Lifo_stack.svg, maybe plus len/is_empty/clear methods. But no change or access of data on the middle. That would be an array or linked list implementation, which is a different story.

Can you also check how the Map type works in this repo? It can be used in const places and is just a configuration how to access the contract storage. I think we should do the same here too.

packages/storage-plus/src/stack.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/stack.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/stack.rs Show resolved Hide resolved
packages/storage-plus/src/stack.rs Show resolved Hide resolved
packages/storage-plus/src/stack.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/stack.rs Show resolved Hide resolved
@uint
Copy link
Contributor

uint commented Aug 24, 2022

Nice job including all the tests.

I don't fully understand the use case for Stack::add_suffix. It looks like you want Stack to be both a stack implementation and a manager/factory of Stacks, all at once?

@itsHaseebSaeed
Copy link
Author

Nice job, including all the tests.

I don't fully understand the use case for Stack::add_suffix. It looks like you want Stack to be both a stack implementation and a manager/factory of Stacks, all at once?

Suffix does what Key() does in Map. Creates new Path

@uint
Copy link
Contributor

uint commented Aug 24, 2022

I'm wondering if it wouldn't make more sense to

  1. implement a Stack that only allows you to have one stack at a fixed prefix, and then
  2. think about some sort of StackMap<K, T> type to aid in managing access to multiple Stacks in a similar way Map helps in managing access to multiple pieces of data.

That way the new types have clearer purposes and the use case of "one value defined as a constant to manage multiple stacks" is still achieved.

With the current Stack implementation there might be a name collision issue too if someone actually tries to add new items to both a "root" Stack and a "derivative" Stack.

@itsHaseebSaeed
Copy link
Author

itsHaseebSaeed commented Aug 25, 2022

I'm wondering if it wouldn't make more sense to

  1. implement a Stack that only allows you to have one stack at a fixed prefix, and then
  2. think about some sort of StackMap<K, T> type to aid in managing access to multiple Stacks in a similar way Map helps in managing access to multiple pieces of data.

That way the new types have clearer purposes and the use case of "one value defined as a constant to manage multiple stacks" is still achieved.

With the current Stack implementation there might be a name collision issue too if someone actually tries to add new items to both a "root" Stack and a "derivative" Stack.

That could be done as well. Right now, you can have or not have a suffix, it's your choice. I'll be happy to do it using Map's way. Just making sure that everyone is on board with the idea, and then I'll do a coding sprint in the coming days. Just keep on adding any other suggestions and ideas as well.

Copy link
Member

@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.

I really like the tests.
Please update README with examples and CHANGELOG.
There are plenty of small stuff to go through - please make sure spacing between methods is one line. Currently sometimes there is none.

packages/storage-plus/src/stack.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/stack.rs Outdated Show resolved Hide resolved
itsHaseebSaeed and others added 4 commits August 26, 2022 16:52
Co-authored-by: Jakub Bogucki <software-solutions@tuta.io>
Co-authored-by: Jakub Bogucki <software-solutions@tuta.io>
@uint
Copy link
Contributor

uint commented Sep 13, 2022

Hey @itsHaseebSaeed! Are you still working on this?

@itsHaseebSaeed
Copy link
Author

Hey @itsHaseebSaeed! Are you still working on this?

@uint funnily enough, I just added this to my weekly to-do list just today. It's will be done max by Friday then I'll start working on deque store

@uint
Copy link
Contributor

uint commented Sep 13, 2022

@uint funnily enough, I just added this to my weekly to-do list just today. It's will be done max by Friday then I'll start working on deque store

Awesome! No hurry, just wanted to check in. LMK if you need help!

@ethanfrey ethanfrey added this to the 0.15.1 milestone Sep 16, 2022
@uint
Copy link
Contributor

uint commented Sep 26, 2022

Hey, @itsHaseebSaeed! We currently have both a need for this functionality and the resources to develop this in-house. Additionally, we might have some ideas around how to make a "map of stacks" functionality better fit our current system. Rather than spam you with change requests, we'd like to take over the implementation. Thank you for your contribution!

@chipshort chipshort mentioned this pull request Sep 26, 2022
@uint
Copy link
Contributor

uint commented Sep 26, 2022

Closing in favor of #807

@uint uint closed this Sep 26, 2022
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

7 participants