-
Notifications
You must be signed in to change notification settings - Fork 75
refactor(l2): refactor privileged transactions #3365
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
Conversation
Lines of code reportTotal lines added: Detailed view
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
|
No significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: Push
|
8c2f401
to
f7e6cae
Compare
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.
Looks good! Just two comments
function getPendingDepositLogs() public view returns (bytes32[] memory) { | ||
return pendingDepositLogs; | ||
function getPendingTransactionHashes() | ||
public | ||
view | ||
returns (bytes32[] memory) | ||
{ | ||
return pendingTxHashes; | ||
} |
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 fully remove this function and use pendingTxHashes()(bytes32[])
instead
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.
Removed in 6ed285b
This reverts commit 6ed285b.
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.
This is just a small modification to enhance the user experience for end users.
event L1ToL2Message( | ||
uint256 indexed amount, | ||
event PrivilegedTxSent ( | ||
address indexed from, |
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 discussed this with @ilitteri and @jrchatruc, and we agreed that we should add an address indexed L1From (the name can be adjusted) so that wallets can track their own privileged transactions for UX purposes.
We also agreed that this should be the only indexed parameter, unless someone can provide a strong reason to keep from, to, or id indexed.
L1From should be set to msg.sender when emitted.
**Description** This PR updates our native bridge documentation with the latest changes, including those from #3365
Motivation
We want to unify the terminology used to refer to sending privileged transactions to the L2, since they are not just deposits.
Also, the
DepositInitiated
(nowPrivilegedTxSent
) event must be cleaned up:l2MintTxHash
is irrelevant since it can be recomputed, and it doesn't make sense to indexamount
but notfrom
.Some clean up (for example, removing
recipient
) was already done in #3320.Description
indexed
tofrom
and removes it fromaddress
Closes #3233