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

Refactor FlowEpoch.getEpochMetadata to avoid the dictionary-copy #384

Closed
SupunS opened this issue Sep 19, 2023 · 2 comments
Closed

Refactor FlowEpoch.getEpochMetadata to avoid the dictionary-copy #384

SupunS opened this issue Sep 19, 2023 · 2 comments
Assignees
Labels
Improvement Performance SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board Technical Debt

Comments

@SupunS
Copy link
Member

SupunS commented Sep 19, 2023

Issue To Be Solved

As mentioned in https://github.com/onflow/flow-core-contracts/pull/382/files/4aea8aa7057c43f7eaf4c8fdf1b08a50db7736b4#r1326279483, in the getEpochMetadata method, the reading of the metadata map from storage was changed from:

self.account.storage.borrow<{UInt64: EpochMetadata}>(from: self.metadataStoragePath)

To

self.account.storage.copy<{UInt64: EpochMetadata}>(from: self.metadataStoragePath)

Because, accessing the inner-object form the "borrow"ed map now returns a reference, where as the rest of the code-base has been written to work the dereferenced-object.

Suggest A Solution

However, this makes this operation very expensive, as it copies the entire map. To avoid this, we would need to either:

  • Support a way to get a de-referenced copy from a reference in Cadence (not supported yet)
  • Update the rest of the contract-code to work with the reference.

It would be good to go with the second option if possible, since that way we don't need to copy anything from the storage (not even the inner EpochMetadata object)

@joshuahannan
Copy link
Member

I think that the second one could work, but we would need to add a second method to get epoch metadata because we can't allow getting a reference to the metadata to be called by everyone. So there would be one private one that uses the reference and one public one that copies it

@joshuahannan joshuahannan self-assigned this Sep 20, 2023
@joshuahannan joshuahannan added the SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board label Sep 20, 2023
@joshuahannan
Copy link
Member

This was fixed in Janez' PR, so I think we can close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Performance SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board Technical Debt
Projects
None yet
Development

No branches or pull requests

2 participants