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.

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

[EVM] Add bridging interface to EVM contract #5677

Merged
merged 13 commits into from
Apr 19, 2024

Conversation

sisyphusSmiling
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling commented Apr 15, 2024

Related: #5663

Adds pre-1.0 bridging interfaces to EVM contract as suggested by @turbolent in #5664

It looks like a number of tests are still failing, though I can't get to the bottom of why. In particular, fvm_test.TestEVM cases appear to be failing on failed contract declaration even though this PR includes updates to alias NFT & FT contracts.

@sisyphusSmiling sisyphusSmiling self-assigned this Apr 15, 2024
@sisyphusSmiling sisyphusSmiling changed the title Giovanni/add pre 1 bridge interface [EVM] Add bridging interface to EVM contract Apr 15, 2024
@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review April 16, 2024 17:47
@sisyphusSmiling sisyphusSmiling added enhancement New feature or request Feature labels Apr 16, 2024
fvm/fvm_test.go Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

Nice work! BTW, I only suggested to have this on master and not only on the Cadence 1.0 feature branch, so the EVM code in the branches is the same, besides Cadence 1.0 compatibility

Copy link
Contributor

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

Nice work 👏

@sisyphusSmiling
Copy link
Contributor Author

Thank you @ramtinms! FYI I added an event and setter in 78efce2 based on feedback from you and @janezpodhostnik. I don't believe we can emit events in interfaces pre-1.0, but I plan on emitting BridgeAccessorUpdated within the new BridgeRouter.setBridgeAccessor method pre-condition in the 1.0 version.

@turbolent turbolent requested a review from sideninja April 17, 2024 19:19
@turbolent turbolent requested a review from a team April 17, 2024 19:19
@turbolent
Copy link
Member

CI fails because test is already broken on master currently

@sisyphusSmiling
Copy link
Contributor Author

CI fails because test is already broken on master currently

Is this an action item for this PR or just a statement saying this is ready for merge?

@turbolent
Copy link
Member

@sisyphusSmiling

CI fails because test is already broken on master currently

Is this an action item for this PR or just a statement saying this is ready for merge?

A test on master is currently broken, so all PRs against master fail too.
I opened #5737 to try and fix it.
See https://discord.com/channels/613813861610684416/1230603448802742362

Once master is fixed, master should be merged into this PR, and then it can be merged once all tests pass.

@sisyphusSmiling
Copy link
Contributor Author

@turbolent thanks for the guidance, master has been merged and it looks like CI is now passing.

@sisyphusSmiling sisyphusSmiling added this pull request to the merge queue Apr 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 19, 2024
@sisyphusSmiling sisyphusSmiling added this pull request to the merge queue Apr 19, 2024
Merged via the queue into master with commit 41926ad Apr 19, 2024
55 checks passed
@sisyphusSmiling sisyphusSmiling deleted the giovanni/add-pre-1-bridge-interface branch April 19, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Feature
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants