[Fix] References in StorageKey#3782
Conversation
src/Neo/SmartContract/StorageKey.cs
Outdated
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="StorageKey"/> class. | ||
| /// </summary> | ||
| /// <param name="cache">The cached byte array. NOTE: It must be read-only and can be modified by the caller.</param> |
There was a problem hiding this comment.
We have the note for this, i thing we create and duplicate without need
There was a problem hiding this comment.
I already shown how it can be changed in the comments and in a test.
Its obvious. |
|
@shargon STOP CHANGING MY CODE. |
what change? |
|
The reason I did that is because yes |
|
@cschuchardt88 Need resolve conflicts |
src/Neo/SmartContract/StorageKey.cs
Outdated
| @@ -255,30 +273,26 @@ public StorageKey() | |||
| /// </summary> | |||
| /// <param name="id">Contract Id</param> | |||
| /// <param name="cache">The cached byte array. NOTE: It must be read-only and can be modified by the caller.</param> | |||
shargon
left a comment
There was a problem hiding this comment.
@cschuchardt88 Serialize should be faster than GetSpan, could you merge this request?
|
I think this PR is OK. |
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Shargon <shargon@gmail.com>
|
@shargon Note that The above it Also I made all your changes. |
| prefix.CopyTo(buffer.AsSpan(sizeof(int))); | ||
| return buffer; | ||
| prefix.CopyTo(buffer[sizeof(int)..]); | ||
| return [.. buffer]; // Copy off stack |
There was a problem hiding this comment.
Why this copy if it's created inside the method?
shargon
left a comment
There was a problem hiding this comment.
Although I do not agree with some of these changes, as agreed in the core developers meeting, I will apply the changes in another pull request
* master: (52 commits) Add SHA512 (#3845) Clean crypto (#3844) Review of #3782 (#3843) [`Fix`] References in `StorageKey` (#3782) Ensure `IInteroperable.Clone` (#3829) Ensure the interop types are serializable before commit (#3802) fix: ModInverse extension in BigIntegerExtensions (#3840) Style: avoid allocating emtpy array (#3841) Style: format json in tests/ (#3839) Reduce mem arguments (#3838) Avoid returning the entire mempool when only `maxTransactionsPerBlock` is required (#3823) style: use proper Assert methods (#3834) Nullable and fix `lock` in `SQLiteWallet` (#3816) [`Add`]: `Witness.Empty` for simplifing somethings (#3836) Style: format too-long literal strings (#3835) Add: NotNullWhen(true) for some out parameters (#3833) Update SQLiteWallet.cs (#3830) Revert "Optimize DeleteAccount" Revert "add lock to Version" Revert "Optimize LoadAccounts" ... # Conflicts: # neo.sln
* Fixed References in StorageKey * Update src/Neo/SmartContract/StorageKey.cs * Update src/Neo/SmartContract/StorageKey.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update src/Neo/SmartContract/StorageKey.cs Co-authored-by: Shargon <shargon@gmail.com> * Update src/Neo/SmartContract/StorageKey.cs Co-authored-by: Shargon <shargon@gmail.com> * Apply suggestions from code review Co-authored-by: Shargon <shargon@gmail.com> * Changed Text --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>
* Review of neo-project#3782 * Clean * Add note
Description
Fixed references in the
StorageKey, now makes copies for memory regions. We can't have data be overwritten. I believe this wasn't intended. Other optimizations like usingSpaninstead ofbyte[]on the stack. Also wrote comments stating and showing the problem with not copying data in the pass. Also wrote tests to make sure data is copied and not referenced.Type of change
How Has This Been Tested?
Checklist: