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(protocol): Additional integration tests, solidity bump, reduce TokenVault contract size #13155

Merged
merged 17 commits into from
Feb 16, 2023

Conversation

cyberhorsey
Copy link
Contributor

@cyberhorsey cyberhorsey commented Feb 15, 2023

This adds some additional integration tests, as well as updates Solidity to 0.8.18 so we can take advantage of the named mapping parameters function, which definitely helps readability on some of our more complex mappings, and removes the need for comments. It also fixes the linter/type errors on the new customError handling functions, as well as hardhat-contract-sizer to get to the bottom of the large contract which is failing our CICD randomly, which turned out to be the TokenVault, which is almost at 24.5kb max contract size. I got it down to 22.7kb with custom errors refactoring and slight modifications elsewhere, and now it seems to not fail our CICD.

However, solhint is not ready for named parameter mappings just yet:
protofire/solhint#397

I can go ahead and undo those changes if we want to wait for solhint to be ready for us before we do this change.

@dantaik as well, in solidity 0.9.0, block.difficulty will be removed from Solidity, which we use for setting the mixHash when proposing a block.

@cyberhorsey cyberhorsey changed the title feat(protocl): Additional integration tests feat(protocol): Additional integration tests Feb 15, 2023
@cyberhorsey cyberhorsey changed the title feat(protocol): Additional integration tests feat(protocol): Additional integration tests, solidity bump Feb 15, 2023
@cyberhorsey cyberhorsey self-assigned this Feb 15, 2023
@cyberhorsey cyberhorsey changed the title feat(protocol): Additional integration tests, solidity bump feat(protocol): Additional integration tests, solidity bump, reduce TokenVault contract size Feb 15, 2023
@dantaik
Copy link
Contributor

dantaik commented Feb 15, 2023

I can go ahead and undo those changes if we want to wait for solhint to be ready for us before we do this change.
No need to care about the solint errors.

@dantaik as well, in solidity 0.9.0, block.difficulty will be removed from Solidity, which we use for setting the mixHash when proposing a block
I'll take a look.

dantaik
dantaik previously approved these changes Feb 15, 2023
@davidtaikocha
Copy link
Contributor

@dantaik as well, in solidity 0.9.0, block.difficulty will be removed from Solidity, which we use for setting the mixHash when proposing a block.

I believe we should use block.prevrandao instead, which was introduced in solidity 0.8.18:

image

@davidtaikocha
Copy link
Contributor

And seems there are some errors in github action (test:tokenomics), hmm not sure if its because of the changes in this PR ?

@dantaik
Copy link
Contributor

dantaik commented Feb 15, 2023

@cyberhorsey I accidentally merged my PR onto yours before fixing test bugs. Please undo the last PR of mine.

@cyberhorsey cyberhorsey marked this pull request as ready for review February 15, 2023 23:16
@cyberhorsey
Copy link
Contributor Author

cyberhorsey commented Feb 15, 2023

@dantaik @davidtaikocha solidity-coverage library is not ready for new solidity features, and the repo is potentially semi-abandoned (hanging open issues and PRs for quite some time).

I have submitted a PR to fix the repo so it works with 0.8.18:
sc-forks/solidity-coverage#783
But I am unsure if it will be merged in. In the meantime, I have changed our dependency to point to my fork. Let me know if that isn't OK with you. I could also fork it under the taikoxyz org, or we could disable coverage.

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #13155 (f8230f9) into main (4f7ab64) will increase coverage by 0.21%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main   #13155      +/-   ##
==========================================
+ Coverage   60.95%   61.16%   +0.21%     
==========================================
  Files         115      115              
  Lines        3391     3394       +3     
  Branches      460      463       +3     
==========================================
+ Hits         2067     2076       +9     
+ Misses       1241     1234       -7     
- Partials       83       84       +1     
Flag Coverage Δ *Carryforward flag
bridge-ui 92.61% <ø> (ø) Carriedforward from f54fa8f
protocol 52.21% <50.00%> (+0.41%) ⬆️
relayer 66.06% <ø> (ø) Carriedforward from f54fa8f
ui 100.00% <ø> (ø) Carriedforward from f54fa8f

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
packages/protocol/contracts/L1/TaikoL1.sol 32.69% <ø> (ø)
...ckages/protocol/contracts/L1/libs/LibProposing.sol 6.57% <ø> (+0.16%) ⬆️
packages/protocol/contracts/L1/libs/LibProving.sol 0.00% <0.00%> (ø)
packages/protocol/contracts/L2/TaikoL2.sol 53.84% <ø> (ø)
packages/protocol/contracts/bridge/EtherVault.sol 76.19% <ø> (ø)
...s/protocol/contracts/bridge/libs/LibBridgeData.sol 100.00% <ø> (ø)
...es/protocol/contracts/test/libs/TestLibProving.sol 0.00% <0.00%> (ø)
...s/protocol/contracts/thirdparty/AddressManager.sol 100.00% <ø> (ø)
...protocol/contracts/thirdparty/ERC20Upgradeable.sol 61.44% <ø> (ø)
packages/protocol/contracts/bridge/TokenVault.sol 51.08% <55.55%> (+0.51%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

davidtaikocha
davidtaikocha previously approved these changes Feb 16, 2023
@dantaik
Copy link
Contributor

dantaik commented Feb 16, 2023

I could also fork it under the taikoxyz org, or we could disable coverage.

I think we can fork the repo under taiko's account.

@cyberhorsey
Copy link
Contributor Author

I could also fork it under the taikoxyz org, or we could disable coverage.

I think we can fork the repo under taiko's account.

OK, done

@dantaik dantaik added this pull request to the merge queue Feb 16, 2023
Merged via the queue into main with commit ffdf5db Feb 16, 2023
@dantaik dantaik deleted the addtl_int_tests branch February 16, 2023 12:03
@github-actions github-actions bot mentioned this pull request Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants