-
Notifications
You must be signed in to change notification settings - Fork 36
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
[TenZen] System contracts deployment flow. #2079
Conversation
Description please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
High-level looks good.
|
||
|
||
|
||
library Structs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also comment pls
import "./OnBlockEndCallback.sol"; | ||
import "./Transaction.sol"; | ||
|
||
//TODO: @PR Review - Pick appropriate name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a comment.
I guess, this is the system contract that will dispatch the txs to the callbacks?
What about PostExecutionProcessor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say rename before merging
return nil, fmt.Errorf("could not process on block end transaction hook. Cause: %w", err) | ||
} | ||
if err = executor.verifySyntheticTransactionsSuccess(onBlockPricedTxes, onBlockSuccessfulTx, onBlockTxResult); err != nil { | ||
// todo: remove once feature finished |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what this is about
} | ||
|
||
s.logger.Debug("CreateOnBatchEndTransaction: Signing transaction", "to", s.transactionsAnalyzerAddress.Hex(), "nonce", nonceForSyntheticTx) | ||
signedTx, err := s.ownerWallet.SignTransaction(tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
being a synthetic tx, does it need to be signed?
If not, you wouldn't neet to pass that wallet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to check on that, don't remember how the evm facade was doing things behind the scenes.
reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another pass
@@ -47,6 +48,42 @@ type BatchHeader struct { | |||
CrossChainTree SerializedCrossChainTree `json:"crossChainTree"` // Those are the leafs of the merkle tree hashed for privacy. Necessary for clients to be able to build proofs as they have no access to all transactions in a batch or their receipts. | |||
} | |||
|
|||
func ConvertBatchHeaderToHeader(batchHeader *BatchHeader) *types.Header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to be a service because it needs access to the secret entropy to calculate the randomness per tx.
contracts/src/system/Transaction.sol
Outdated
uint256 value; | ||
bytes data; | ||
address from; | ||
bool successful; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs more fields from the receipt. Like how much gas it used. It might be useful
import "./OnBlockEndCallback.sol"; | ||
import "./Transaction.sol"; | ||
|
||
//TODO: @PR Review - Pick appropriate name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say rename before merging
@@ -47,6 +48,42 @@ type BatchHeader struct { | |||
CrossChainTree SerializedCrossChainTree `json:"crossChainTree"` // Those are the leafs of the merkle tree hashed for privacy. Necessary for clients to be able to build proofs as they have no access to all transactions in a batch or their receipts. | |||
} | |||
|
|||
func ConvertBatchHeaderToHeader(batchHeader *BatchHeader) *types.Header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised what the problem is. We don't expose the converted ethereum headers and that's why all this conversion.
We even expose them in the gateway and there is nothing to back it up.
Pls raise a ticket to do this properly, and add a todo here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Few minor things. And pls add the pr description
Why this change is needed
Please provide a description and a link to the underlying ticket
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks