Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

Storage Dynamic Arrays #1035

Closed
wants to merge 15 commits into from
Closed

Storage Dynamic Arrays #1035

wants to merge 15 commits into from

Conversation

AlejandroLabourdette
Copy link
Contributor

@AlejandroLabourdette AlejandroLabourdette commented Apr 17, 2023 •
edited
Loading

Issue: #1026.

@AlejandroLabourdette AlejandroLabourdette changed the base branch from develop to cairo-1.0 April 17, 2023 17:24
@AlejandroLabourdette AlejandroLabourdette marked this pull request as draft April 17, 2023 17:34
@AlejandroLabourdette AlejandroLabourdette marked this pull request as ready for review April 18, 2023 07:41
@AlejandroLabourdette AlejandroLabourdette changed the title Dynamic arrays initial support Dynamic arrays Apr 18, 2023
rodrigo-pino
rodrigo-pino reviewed Apr 21, 2023
View reviewed changes
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

It looks good to me. Minor comments and it's good to merge. Could you please remove the changes you did to warplib except for integers. They are not very important for this PR and it will break other branches working with memory.,

src/cairoUtilFuncGen/storage/dynArrayPushWithArg.ts Outdated Show resolved Hide resolved
src/cairoUtilFuncGen/storage/dynArrayPushWithArg.ts Outdated Show resolved Hide resolved
src/cairoUtilFuncGen/storage/dynArrayPushWithoutArg.ts Outdated Show resolved Hide resolved
src/cairoUtilFuncGen/storage/dynArrayPushWithoutArg.ts Outdated Show resolved Hide resolved
@rodrigo-pino rodrigo-pino changed the title Dynamic arrays Storage Dynamic Arrays Apr 21, 2023
@piwonskp
Copy link
Contributor

Conflicts

@AlejandroLabourdette
Copy link
Contributor Author

AlejandroLabourdette commented Apr 24, 2023 •
edited
Loading

t looks good to me. Minor comments and it's good to merge. Could you please remove the changes you did to warplib except for integers. They are not very important for this PR and it will break other branches working with memory.,

Actually they are, we need to have those memory warplib functions working to be able to handle storage dynamic arrays for example. Initial issue #1026 was just to be able to create memory dynamic arrays, but it wouldn't do much if we can't access or update them. Also seems logic to include support for storage dynamic arrays in this pr.
All old Cairo0 code from memory.cairo was added to memory.old.cairo file and in the memory.cairo were left just the Cairo1 equivalent of the used util functions. The end will be to translate utils functions as they are needed or perhaps a separate issue to translate all at once (I prefer first approach because they are wide thematic and would be hard to test) and then delete that memory.old.cairo file

piwonskp
piwonskp reviewed Apr 24, 2023
View reviewed changes
warplib/src/warp_memory.cairo
@@ -28,10 +29,14 @@ impl WarpMemoryImpl of MemoryTrait {

fn insert(ref self: WarpMemory, position: felt252, value: felt252) {
self.memory.insert(position, value);
self.pointer += 1;
Copy link
Contributor

@piwonskp piwonskp Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The change breaks the logic in arrayLiteral.ts. Please consider the insert call there

Copy link
Contributor Author

@AlejandroLabourdette AlejandroLabourdette Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this still an issue @piwonskp after what we discuss in daily?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not anymore, now we just should use create for static arrays

@AlejandroLabourdette AlejandroLabourdette marked this pull request as draft May 17, 2023 01:27
@rodrigo-pino rodrigo-pino deleted the dyn_arrays branch June 8, 2023 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rodrigo-pino rodrigo-pino rodrigo-pino left review comments

@piwonskp piwonskp piwonskp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AlejandroLabourdette @piwonskp @rodrigo-pino