fix(protocol): address miscellaneous feedbacks from Sigma Prime (TKO26) #15600
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dantaik
requested changes
Jan 29, 2024
Brechtpd
reviewed
Jan 29, 2024
There was a problem hiding this comment.
- I would move LibUint512Math and TaikoL2Signer to the test folder, they are only used in the tests. Only
GOLDEN_TOUCH_ADDRESSis actually used and so can just be copied toTaikoL2.sol. - We may want to update the Merkle trie/RLP decoding to the latest versions used in optimism: https://github.com/ethereum-optimism/optimism/tree/develop/packages/contracts-bedrock/src/libraries. I haven't diffed them, but seems likely there are also some optimizations in there. In any case, we should definitely check if the new versions contain fixes or improvements.
dantaik
requested changes
Jan 30, 2024
See this PR: #15604
I'll work on this one later. |
Addressed this here too. And all the changes you were recommending. |
dantaik
changed the title
dantaik
reviewed
Jan 30, 2024
dantaik
reviewed
Jan 30, 2024
dantaik
requested changes
Jan 30, 2024
dantaik
reviewed
Jan 30, 2024
dantaik
approved these changes
Jan 30, 2024
Brechtpd
approved these changes
Jan 30, 2024
This was referenced Feb 9, 2024
Closed
This was referenced Apr 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please see this modification + some of my concerns/comments in the notion file at raw TKO_26!
#15600
Some notes to subpoints:
Nr. 6 Unnecessary functions in SignalService and AddressManager. → It would create a diff design scheme we followed, and i understand it is deployment overhead, but would involve to change OwnerUUPS and others inheriting from it OR NOT inheriting from that contract but a similar, without having the pasue(), unPause().
Nr. 13: Cannot find this part:
” [85-87] of LibVerifying.sol contain inequalities where we have for an unsigned inte-
ger x which will never return true. “
Nr. 15.: Total Supply Of TKO Token Difficult To Determine
@daniel Wang
As burning is not an actual burn (in our contract and complex code, i dont exactly know what is the best was to decouple)
Nr. 19: Related Asset(s): L1/hooks/AssignmentHook.sol
It can if tip is, high, but tip is set by the proposer, why would anyone want to give such (unrelaistic amount), they would just shoot in their legs, because it even has to send it with msg.value.
Nr. 21:
Provers May Contest Their Own Proof
I do not see anyone benefitting from this. Contest bond shall be big enough.
Also if some may submitted (accidentally ?) a wrong proof, he (the original prover) can correct it’s mistake by contesting and re-proving with a higher tier, so in such case it does not lose all his bonds.