chore: fix system tx inputs#50
Merged
dapplion merged 5 commits intognosischain:withdrawals-pullfrom Jul 11, 2023
Merged
Conversation
filoozom
requested review from
Barichek and
dapplion
and removed request for
Barichek
dapplion
approved these changes
Jun 28, 2023
dapplion
requested changes
Jun 28, 2023
contracts/SBCDepositContract.sol
Outdated
| } | ||
|
|
||
| /** | ||
| * @custom:deprecated |
Member
There was a problem hiding this comment.
We cannot deprecate this function without breaking chiado. Client teams should be aware of this and must support this signature until we have a plan for chiado
Contributor
Author
There was a problem hiding this comment.
So what change are you suggesting? For me "deprecated" just means that this should no longer be used in the future, but blockchain being blockchain, this is still the signature that needs to be called before some still undefined block on Chiado.
Member
There was a problem hiding this comment.
deprecated does not convey enough meaning and can lead to confusion. Being this so critical it must be explicitly said that:
- chiado requires this signature from block N to block M, supports both after block M
- gnosis supports both after block P
dapplion
approved these changes
Jul 11, 2023
dapplion
approved these changes
Jul 11, 2023
dapplion
added a commit
that referenced
this pull request
Jul 11, 2023
* Withdrawals pull * Update maxFailedWithdrawalsToProcess variable name * Delete settings.json * Place all variables at the top * Update executeSystemWithdrawals comment * chore: fix all solhint issues (#49) * chore: remove global imports * chore: ignore _deprecatedUnused * chore: re-add _deprecatedUnused comment * chore: fix system tx inputs (#50) * chore: add system tx with right signature * fix: make signature public and fix tests * Update SBCDepositContract comment * Update SBCDepositContract.sol --------- Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com> --------- Co-authored-by: Philippe Schommers <philippe@schommers.be>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This needs to be deployed before the merge gets deployed or it might prevent nodes from syncing without specific checks.
This will not work on Chiado without any specific exceptions though, so we might not want to merge this for mainnet, as it would mean that we'd either have to keep the deprecated signature anyways, or implement an override in the EL clients.