refactor: move GetConsecutiveValues functionality to memory package

[MoigeMatino]

What does this PR do?

• Moves the GetConsecutiveValues functionality from hinter package to memory package.
• Makes necessary changes to files that invoked GetConsecutiveValues to match new function location.

Related Issue(s)?

• Resolves #384

[rodrigo-pino]

This doesn't work. Will run CI this time for you to see. Before submiting a PR for review make sure tests are passing. All information is in the Readme. If something is not clear please let me know

[MaksymMalicki]

Hi @MoigeMatino, are you still working on this pull request? I would kindly ask you to let us know. Thanks! 😃

[MoigeMatino]

Hi @MaksymMalicki. Yes, created this PR and asked @TAdev0 for his help resolving some errors I encountered🌸

[rodrigo-pino]

Hey @MoigeMatino could you please move all discussions regarding the issue to Github. This way the whole community is aware of the process of the PR.

[MoigeMatino]

Hey @MoigeMatino could you please move all discussions regarding the issue to Github. This way the whole community is aware of the process of the PR.

@rodrigo-pino I'm afraid I don't understand what you mean. Please elaborate.

[TAdev0]

@MoigeMatino hey, sorry i cannot find where you asked for help. Is it on telegram?

[rodrigo-pino]

Hi @MaksymMalicki. Yes, created this PR and asked @TAdev0 for his help resolving some errors I encountered🌸

@MoigeMatino I gather from this comment that you asked questions/had a discusions about the PR somewhere different than here, which is great! It would be perfect if you could write them here so the whole community can be aware of them. Your question might be someone else questions and also the whole team is aware you are working on this PR.

[MoigeMatino]

Hi @MaksymMalicki. Yes, created this PR and asked @TAdev0 for his help resolving some errors I encountered🌸

@MoigeMatino I gather from this comment that you asked questions/had a discusions about the PR somewhere different than here, which is great! It would be perfect if you could write them here so the whole community can be aware of them. Your question might be someone else questions and also the whole team is aware you are working on this PR.

Okay, I get it now, thanks! Will do that👍🏾. Had the discussion on the issue relating to this PR. This is noted👌🏾. I'll loop @TAdev0 in.

[MoigeMatino]

Hi @TAdev0 🌸, could you kindly assist with the errors I encountered when running make unit? Not sure how to resolve this error:
Error: Expected nil, but got: &errors.errorString{s:"segment 1: unallocated"}. Could be because I called InitializeEmptyMemory before callingGetConsecutiveValues?

[MaksymMalicki]

Hi @TAdev0 🌸, could you kindly assist with the errors I encountered when running make unit? Not sure how to resolve this error: Error: Expected nil, but got: &errors.errorString{s:"segment 1: unallocated"}. Could be because I called InitializeEmptyMemory before callingGetConsecutiveValues?

Hi, for sure you cannot use InitializeEmptyMemory. The more likely solution is to use Memory field from the VM, which hold the actual memory. So you use it like this: vm.Memory.GetConsecutiveValues(pointAddr, int16(6)). Try this out and let us know if you had any progress

[TAdev0]

@MoigeMatino your unit tests dont pass. Please make sure to test locally before pushing. If you have difficulties to make them pass locally, let us know so we can help you on this

[TAdev0]

@MoigeMatino i tested locally and it works well. The reason why your tests fail is that you modified the number of consecutive values to read. For example, you replaced :

hinter.GetConsecutiveValues(vm, slopeAddr, int16(3))

with

vm.Memory.GetConsecutiveMemoryValues(slopeAddr, int16(6))

You did this in 7 places. Because you modified for a greater value, you get the error 'reading unknown value'.

You should ONLY remove the vm argument , and modify hinter for vm.Memory , thats all

[MoigeMatino]

@TAdev0 thanks for the pointers, addressed the comments and pushed changes.

[TAdev0]

LGTM!

[TAdev0]

@rodrigo-pino @cicr99 ready for merge