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

feat(Smart contracts): Fix solidity version + more tests #633

Merged
merged 8 commits into from
Mar 3, 2023

Conversation

cuongquangnam
Copy link
Contributor

@cuongquangnam cuongquangnam commented Mar 2, 2023

What this PR does / why we need it:

  • Fix Solidity version of smart contracts
  • Add more tests for Timelock
  • Reorder code (some for readability, some for best practices)

Which issue(s) does this PR fixes?:

Fixes #293

Additional comments?:

Developer Checklist:

  • Read your code changes at least once
  • No console errors on web
  • Tested on Light mode and Dark mode*
  • Your UI implementation visually matched the rendered design*
  • Unit tests*
  • Added e2e tests*
  • Added translations*

@netlify
Copy link

netlify bot commented Mar 2, 2023

Deploy Preview for quantumbridge ready!

Name Link
🔨 Latest commit 0f345de
🔍 Latest deploy log https://app.netlify.com/sites/quantumbridge/deploys/64014f6b799380000885e10b
😎 Deploy Preview https://deploy-preview-633--quantumbridge.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cuongquangnam cuongquangnam changed the title Cuong/cleanup feat(Smart contracts): Fix solidity version + more tests Mar 2, 2023
@github-actions github-actions bot added the kind/feature New feature request label Mar 2, 2023
25 * 60 * 60,
),
).to.revertedWith('TimelockController: operation already scheduled');
});

it('Change the salt, the operation should be successfully scheduled and later executed', async () => {

Choose a reason for hiding this comment

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

Would it revert if we use the same salt? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, so the salt is calculated this way:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.1/contracts/governance/TimelockController.sol#L194
so let's say if target, value, data, predecessor, salt are the same --> we get a hash that has been scheduled or done --> getTimestamp(hash) > 0 --> schedule will revert (
the check over here

Copy link

@weiyuan95 weiyuan95 left a comment

Choose a reason for hiding this comment

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

This LGTM - not sure if there are any more changes.

@cuongquangnam cuongquangnam temporarily deployed to AWS ECR March 2, 2023 15:13 — with GitHub Actions Inactive
@cuongquangnam cuongquangnam temporarily deployed to AWS ECR March 3, 2023 01:37 — with GitHub Actions Inactive
Copy link
Contributor

@Abhishekkochar Abhishekkochar left a comment

Choose a reason for hiding this comment

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

LGTM

@Abhishekkochar Abhishekkochar requested a review from weiyuan95 March 3, 2023 02:35
@weiyuan95 weiyuan95 merged commit 5d4bfb9 into main Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade solidity version to 0.8.18
3 participants