Skip to content
This repository was archived by the owner on Jun 16, 2025. It is now read-only.

Conversation

fergarrui
Copy link
Member

@fergarrui fergarrui commented May 6, 2025

  • set core bridge messageFee to > 0 for integration tests
  • set core bridge messageFee to > 0 for unit tests
  • fixed placeMarketOrder & placeFastMarketOrder for EVM

@fergarrui fergarrui requested review from gator-boi and a5-pickle May 6, 2025 16:52
Copy link
Contributor

@a5-pickle a5-pickle left a comment

Choose a reason for hiding this comment

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

Left one comment that I'd like to see a response to. Otherwise looks good w.r.t. Token Router market orders and Matching Engine's execute order.

@fergarrui it would be nice to have the same sort of forge test(s) that demonstrate that 2x message fee would be required for the fast market order (since there are two Wormhole messages required to execute).

As far as the Token Router market order, I feel like this should be a forge test instead of in the typescript test just to be consistent with the other tests. I don't have a strong opinion here.

cc: @gator-boi in case you had any thoughts.

@@ -1,5 +1,5 @@
export AVALANCHE_RPC=https://api.avax.network/ext/bc/C/rpc
export ETHEREUM_RPC=https://rpc.ankr.com/eth
export ETHEREUM_RPC=https://eth.llamarpc.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Ankr RPC not work anymore or is it flaky? Just curious how you chose this RPC provider as opposed to another public one.

Copy link
Member Author

@fergarrui fergarrui May 6, 2025

Choose a reason for hiding this comment

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

Ankr free tier is not supported anymore. Chose that one from chainlist, personally used it for other scripts, but we can choose a different one too

@fergarrui
Copy link
Member Author

fergarrui commented May 7, 2025

Left one comment that I'd like to see a response to. Otherwise looks good w.r.t. Token Router market orders and Matching Engine's execute order.

@fergarrui it would be nice to have the same sort of forge test(s) that demonstrate that 2x message fee would be required for the fast market order (since there are two Wormhole messages required to execute).

As far as the Token Router market order, I feel like this should be a forge test instead of in the typescript test just to be consistent with the other tests. I don't have a strong opinion here.

cc: @gator-boi in case you had any thoughts.

@a5-pickle

Are there existing tests already for that flow where 2 messages are sent?
I did set messageFee to > 0 in unit tests but all of them are passing using msg.value == messageFee().
There is an existing fast market order test which seems to send just 1 message. Is this the flow that sends 2 messages?

Thanks!

@a5-pickle
Copy link
Contributor

Left one comment that I'd like to see a response to. Otherwise looks good w.r.t. Token Router market orders and Matching Engine's execute order.
@fergarrui it would be nice to have the same sort of forge test(s) that demonstrate that 2x message fee would be required for the fast market order (since there are two Wormhole messages required to execute).
As far as the Token Router market order, I feel like this should be a forge test instead of in the typescript test just to be consistent with the other tests. I don't have a strong opinion here.
cc: @gator-boi in case you had any thoughts.

@a5-pickle

Are there existing tests already for that flow where 2 messages are sent? I did set messageFee to > 0 in unit tests but all of them are passing using msg.value == messageFee(). There is an existing fast market order test which seems to send just 1 message. Is this the flow that sends 2 messages?

Thanks!

I actually meant this workflow in the Token Router: https://github.com/wormhole-foundation/example-liquidity-layer/blob/main/evm/src/TokenRouter/assets/PlaceMarketOrder.sol#L144-L221.

Check out the TokenRouter.t.sol file for calls to placeFastMarketOrder. Let me know if you have any issues.

@fergarrui
Copy link
Member Author

Left one comment that I'd like to see a response to. Otherwise looks good w.r.t. Token Router market orders and Matching Engine's execute order.
@fergarrui it would be nice to have the same sort of forge test(s) that demonstrate that 2x message fee would be required for the fast market order (since there are two Wormhole messages required to execute).
As far as the Token Router market order, I feel like this should be a forge test instead of in the typescript test just to be consistent with the other tests. I don't have a strong opinion here.
cc: @gator-boi in case you had any thoughts.

@a5-pickle
Are there existing tests already for that flow where 2 messages are sent? I did set messageFee to > 0 in unit tests but all of them are passing using msg.value == messageFee(). There is an existing fast market order test which seems to send just 1 message. Is this the flow that sends 2 messages?
Thanks!

I actually meant this workflow in the Token Router: https://github.com/wormhole-foundation/example-liquidity-layer/blob/main/evm/src/TokenRouter/assets/PlaceMarketOrder.sol#L144-L221.

Check out the TokenRouter.t.sol file for calls to placeFastMarketOrder. Let me know if you have any issues.

I see what you mean, true, I added the same test for TokenRouter.

@nik-suri nik-suri requested a review from a5-pickle May 14, 2025 21:03
@a5-pickle a5-pickle changed the title Use messageFee > 0 evm: use messageFee > 0 May 17, 2025
@a5-pickle a5-pickle merged commit 3879b3c into main May 17, 2025
1 check passed
@a5-pickle a5-pickle deleted the evm/core-bridge-message-fee branch May 17, 2025 14:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants