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

Adds bridge to fvm bootstrapping #7053

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

joshuahannan
Copy link
Member

Adds the bridge standup script to fvm bootstrapping so that the bridge can be available in the emulator and other testing environments by default

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 57.53647% with 262 lines in your changes missing coverage. Please review.

Project coverage is 41.29%. Comparing base (82d7f1d) to head (30cb755).

Files with missing lines Patch % Lines
fvm/blueprints/bridge.go 0.00% 229 Missing ⚠️
fvm/blueprints/token.go 0.00% 20 Missing ⚠️
fvm/bootstrap.go 98.30% 4 Missing and 2 partials ⚠️
fvm/systemcontracts/system_contracts.go 61.53% 5 Missing ⚠️
utils/unittest/execution_state.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7053      +/-   ##
==========================================
+ Coverage   41.25%   41.29%   +0.03%     
==========================================
  Files        2170     2171       +1     
  Lines      190050   190632     +582     
==========================================
+ Hits        78409    78722     +313     
- Misses     105097   105361     +264     
- Partials     6544     6549       +5     
Flag Coverage Δ
unittests 41.29% <57.53%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joshuahannan joshuahannan force-pushed the add-bridge-to-bootstrapping branch 2 times, most recently from 12adf5e to 7ae57c9 Compare February 25, 2025 14:52
@joshuahannan joshuahannan force-pushed the add-bridge-to-bootstrapping branch 2 times, most recently from ba6bf93 to 0d96420 Compare March 7, 2025 19:45
@joshuahannan joshuahannan marked this pull request as ready for review March 7, 2025 19:45
@joshuahannan
Copy link
Member Author

I finished writing all my tests so it is ready for review!

@janezpodhostnik There are still some tests failing that I'm confused about. In the evm tests, the bootstrapping is failing with this error:

    --- FAIL: TestEVMBatchRun/Batch_run_multiple_valid_transactions (0.68s)
panic: failed to set the factory as registrar: [Error Code: 1101] error caused by: 1 error occurred:
		* transaction execute failed: [Error Code: 1101] cadence runtime error: Execution failed:
	  --> 512aac31a246aa175e87cf5e9cf7334d69457540e034e85feeca351e80b07a80:44:28
	   |
	44 |         let decodedResult = EVM.decodeABI(
	45 |                 types: [Type<EVM.EVMAddress>()],
	46 |                 data: postRegistrarResult.data
	47 |             )
	   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	
	error: failed to ABI decode data
	   --> f8d6e0586b0a20c7.EVM:622:8

Do you know why the ABI decoding would fail in that test but not in any of the fvm tests?

@joshuahannan joshuahannan force-pushed the add-bridge-to-bootstrapping branch from 0d96420 to 2ae0bed Compare March 10, 2025 15:01
@janezpodhostnik
Copy link
Contributor

Thank you very much for preparing this! I will take a look at the error in a couple of days.

@joshuahannan joshuahannan force-pushed the add-bridge-to-bootstrapping branch from 2ae0bed to 976d09d Compare March 11, 2025 16:54
@joshuahannan
Copy link
Member Author

Thanks, just to provide a bit more context, that is coming from the bridge setup part of bootstrapping that I wrote. It is from the step where we provide the address of the registrar contract to set the token factory contract that deploys the bridge deployed solidity token smart contracts.

It happens when we are decoding the ABI data from the registrar contract.

This step succeeds in all of the fvm tests, but doesn't in the evm tests, so it sounds to me like maybe there is something in the config for the EVM tests that differs from the FVM tests that makes this not work in this context.

@janezpodhostnik
Copy link
Contributor

What I found is that there is an underlying error: "abi: attempting to unmarshal an empty string while arguments are expected"

at this location: https://github.com/onflow/flow-evm-bridge/blob/b32c787631c9c84c548b2e414d87b49fa32b12d8/cadence/transactions/bridge/admin/evm/set_registrar.cdc#L44

which means the postRegistrarResult.data was empty... but I don't know what that means yet.

@joshuahannan
Copy link
Member Author

Interesting. @sisyphusSmiling Are you able to take a look? Do you know why postRegistrarResult.data would be empty? I would assume that it being empty would mean that the registrar address was not set properly, but I also figured that one of the previous EVM transactions in the Cadence transaction would have failed first. What do you think?

@janezpodhostnik
Copy link
Contributor

Looks like evm_test are written a bit weird and are mixing 2 different storages, so the bootstrapping does not start with an empty storage.

They will need to be changed to be more like the fvm_tests.

@joshuahannan
Copy link
Member Author

@janezpodhostnik Just to clarify, how would starting with a different storage affect the bridge bootstrapping? And are you saying that you'll be able to help update them, or I should do that?

@janezpodhostnik
Copy link
Contributor

I am trying to update the tests, but I'm unfamiliar with these tests and its proving difficult. I will let you know how it goes.

@janezpodhostnik
Copy link
Contributor

@joshuahannan I tried fixing the tests, but it would take me more time than I currently have.

I suggest adding a disable option for bootstrapping the bridge, and disable the bridge in these specific tests (evm_test.go).

@joshuahannan
Copy link
Member Author

okay, I'll do that. Thanks for taking a look!

@joshuahannan joshuahannan force-pushed the add-bridge-to-bootstrapping branch from 976d09d to 30cb755 Compare March 20, 2025 16:12
@joshuahannan
Copy link
Member Author

@janezpodhostnik I just pushed the changes and everything is passing now. Can you review? Also, since I added the bootstrap option to setup the bridge and it doesn't happen by default any more, where do we add that to use that for the bootstrapping that is used for the emulator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants