Skip to content

Eth contract fix memory + new sdk + refactor #257

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

Merged
merged 9 commits into from
Aug 24, 2022

Conversation

ali-behjati
Copy link
Collaborator

  • Change some memory modifiers to improve gas efficiency
  • Implement getValidTimePeriod() and remove old staleness logic
  • Update the tests

- Change some memory modifiers to improve gas efficiency
- Implement getValidTimePeriod() and remove old staleness logic
- Update the tests
@ali-behjati ali-behjati requested review from jayantk and tompntn and removed request for jayantk August 24, 2022 11:33
@ali-behjati ali-behjati requested a review from drozdziak1 August 24, 2022 12:05
@ali-behjati
Copy link
Collaborator Author

I have used a placeholder bytes and i'm replacing it within the deployment process I replace the placeholder with the hash in the contract binary.. So it's not part of the contract and return hex"123123..." doesn't look quite clean.

@tompntn raised it and suggested to use a storage variable instead, and a method like setGitCommitHash() to set as the migration call in the upgradeProxy. However, I think it's too much to have it as a migration call. We are likely to use migrate functions and then in each of them we should make sure this hash is set correctly. Also, the nature of this hash is not a variable to be set. In the placeholder version it's within the implementation code and won't change (tied to implementation). In the storage variable it's in the proxy storage and will change every time.

I decided to move on with the existing placeholder approach (although it doesn't look nice), I am not particularly happy with it. We definitely can revisit it. We can change to migration call whenever needed in the future too and we just need to change the underlying value of deployCommitHash.

Posted a summary here for future reference when we get back to it.

@ali-behjati ali-behjati merged commit a17b27f into main Aug 24, 2022
@ali-behjati ali-behjati deleted the abehjati/fix-eth-contract-mem-layout-bug branch August 24, 2022 17:33
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