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.

Sign up for GitHub

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

Jump to bottom

Feat(exit precompile): Events for exit precompile calls. #288

Merged
merged 6 commits into from
Oct 1, 2021
Merged

Feat(exit precompile): Events for exit precompile calls. #288

merged 6 commits into from
Oct 1, 2021

Conversation

birchmd
Copy link
Member

@birchmd birchmd commented Sep 28, 2021

Closes #245 (see that ticket for details)

@paouvrard mentioned that an explicit event for ERC-20 exit, was not required, but it seemed easy to me to include it now, so I did.

This PR includes new tests showing the events are created.

@birchmd birchmd added C-enhancement Category: New feature or request A-precompiles Area: Issues that relate to the precompiles. labels Sep 28, 2021
@birchmd birchmd linked an issue Sep 28, 2021 that may be closed by this pull request
feat: Emit Aurora event in ETH exit precompile. #245
Closed
sept-en
sept-en approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@sept-en sept-en left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Thanks @birchmd !

@paouvrard
Copy link
Member

@birchmd This should work for the frontend but I realized it would also be useful to filter by sender and token. Is it possible to include the sender and the erc20 address in the event ?
That would also make it consistent with the Locked event in rainbow bridge: https://github.com/aurora-is-near/rainbow-token-connector/blob/134b3b6ad900d90c6f993c52f4e3edf985aec61a/erc20-connector/contracts/ERC20Locker.sol#L15

mfornet
mfornet suggested changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@mfornet mfornet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

However I agree with @paouvrard about using the extended version of the event.
Marking Request changes to avoid overlooking that!

@birchmd
Copy link
Member Author

birchmd commented Sep 30, 2021 •
edited
Loading

@mfornet @paouvrard

I don't think we can get the sender to include in the event. The issue is that the sender is two steps back in the call stack, not the previous step as with the lockup. This is because the flow is sender -> erc20.withdraw -> exit_precompile. So at the level of the exit precompile, msg.sender returns the address of the ERC-20 token on Aurora, not the user/contract which made the withdraw call.

I can include the ERC-20 address though. I will replace the bool indexed is_erc20 in the current event with Address indexed erc20_address, and in the ETH withdraw case I will set this equal 0x00. Does that work for you? (Added in 7e4c5ad )

mfornet
mfornet approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@mfornet mfornet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can get the sender to include in the event. The issue is that the sender is two steps back in the call stack, not the previous step as with the lockup.

I see

@paouvrard
Copy link
Member

Understood, thanks @birchmd ! It shouldn't be a problem for erc20 because we can always filter with the burn event to get the sender.

But for exits of ETH, the sender calls the exit_precompile directly so it should be possible include it in this case right ?

joshuajbouw
joshuajbouw approved these changes Oct 1, 2021
View reviewed changes
Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. No concerns here. Code quality and effort is excellent. Thanks.

@joshuajbouw
Copy link
Contributor

Just FYI, these should only be staged to testnet first, followed by mainnet in next release.

@joshuajbouw joshuajbouw merged commit 7b263b2 into develop Oct 1, 2021
@joshuajbouw joshuajbouw deleted the 245-eth-exit-event branch October 1, 2021 06:47
@birchmd birchmd mentioned this pull request Oct 1, 2021
Merged
@birchmd
Copy link
Member Author

birchmd commented Oct 1, 2021

@paouvrard Yes, good point. Followed up in #292

birchmd added a commit that referenced this pull request Oct 4, 2021
@birchmd
Feat(exit precompile): Events for exit precompile calls. (#288)
2869601 
* Exit precompiles create events

* Do not filter out exit precompile events

* Refactor exit precompiles test

* Expand exit precompiles test to include looking at the generated event

* Add test for exit to ethereum precompile

* Include ERC-20 address in exit events
birchmd added a commit to birchmd/aurora-engine that referenced this pull request Oct 4, 2021
@birchmd
Feat(exit precompile): Events for exit precompile calls. (aurora-is-n…
239a3e6 
…ear#288)
This was referenced Oct 8, 2021
Closed
Merged
artob pushed a commit that referenced this pull request Oct 15, 2021
@birchmd @dependabot
Release 1.7.0. (#304)
f1ebb82 
* Feat(exit precompile): Events for exit precompile calls. (#288)
* Bump path-parse from 1.0.6 to 1.0.7 in /etc/eth-contracts (#229)
* Include sender field in exit precompile events (#292)
* Feat(tests): ecpair (#296)
* Feat(tests): Uniswap V3 tests and benchmarks (#293)
* Feat(tests): deploying the largest allowed contract (#294)
* Fix(precompiles): Derive Default impl for PrecompileOutput (#295)
* Feat(docs): Benchmark documentation (#298)

Co-authored-by: Michael Birch <michael@aurora.dev>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@paouvrard paouvrard mentioned this pull request Oct 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrLSD mrLSD mrLSD approved these changes

@sept-en sept-en sept-en approved these changes

@joshuajbouw joshuajbouw joshuajbouw approved these changes

@mfornet mfornet mfornet approved these changes

@artob artob Awaiting requested review from artob

@paouvrard paouvrard Awaiting requested review from paouvrard

Assignees
No one assigned
Labels
A-precompiles Area: Issues that relate to the precompiles. C-enhancement Category: New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Emit Aurora event in ETH exit precompile.
6 participants
@birchmd @paouvrard @joshuajbouw @mrLSD @sept-en @mfornet