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

Add the possibility to specify an offset for the read value in StorageRead::read #863

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

acerone85
Copy link
Contributor

[Link to related issue(s) here, if any]

[Short description of the changes.]

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@acerone85 acerone85 self-assigned this Oct 24, 2024
@acerone85 acerone85 marked this pull request as draft October 24, 2024 20:46
@acerone85 acerone85 marked this pull request as ready for review October 28, 2024 10:49
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

LGTM!=) Do I understand correct, that after this change is merged, you will rework #847 ?

Comment on lines 203 to 206
key: &<Type as Mappable>::Key,
buf: &mut [u8],
) -> Result<Option<usize>, T::Error> {
self.0.read(key, buf)
self.0.read(key, 0, buf)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can specify offset here as well or add read_with_offset=)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here: b2335f9
Is there any reason why StorageRef does not implement StorageRead btw? The only thing I see is that it does not implement StorageSize, which is a requirement for StorageRead.

Copy link
Collaborator

@xgreenx xgreenx Oct 30, 2024

Choose a reason for hiding this comment

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

It doesn't implement any traits because it is used like storage::<Mapable>().read without importing the trait(just calling of the function). This structure is used as a syntax sugar to simplify the work within the code

Comment on lines 36 to 38
pub use memory::MemoryStorage;
#[cfg(feature = "test-helpers")]
pub use memory::MemoryStorageError;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub use memory::MemoryStorage;
#[cfg(feature = "test-helpers")]
pub use memory::MemoryStorageError;
pub use memory::{
MemoryStorage,
MemoryStorageError,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed here: 8855eab

CHANGELOG.md Outdated
Comment on lines 14 to 15
- [863](https://github.com/FuelLabs/fuel-vm/pull/863): Changed StorageRead::read to load a serialized value starting from a offset. The function returns an optional value equal to the number of bytes read when defined, or none if the offset specified in input is outside the boundaries of the serialized value read.
## [Version 0.58.2]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- [863](https://github.com/FuelLabs/fuel-vm/pull/863): Changed StorageRead::read to load a serialized value starting from a offset. The function returns an optional value equal to the number of bytes read when defined, or none if the offset specified in input is outside the boundaries of the serialized value read.
## [Version 0.58.2]
- [863](https://github.com/FuelLabs/fuel-vm/pull/863): Changed StorageRead::read to load a serialized value starting from a offset. The function returns an optional value equal to the number of bytes read when defined, or none if the offset specified in input is outside the boundaries of the serialized value read.
## [Version 0.58.2]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changed in 5b91da4

@acerone85
Copy link
Contributor Author

LGTM!=) Do I understand correct, that after this change is merged, you will rework #847 ?

Yes, I'll start working on that shortly. IIUC After the release of the next fuel-vm version I'll also have to update fuel-core to use the new storage?

@xgreenx
Copy link
Collaborator

xgreenx commented Oct 30, 2024

After the release of the next fuel-vm version I'll also have to update fuel-core to use the new storage?

Yes

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.

2 participants