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

feat(protocol): introduce message.processor #16999

Closed
wants to merge 95 commits into from
Closed

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented May 5, 2024

Allow the Bridge UI to set message.processor so only this address and message.destOwner can process the message on the destination chain.

This reverts commit b86fd6b.
@KorbinianK
Copy link
Contributor

Should this be configurable by the user or fixed behind the scene to our relayer? If the first, this requires changes to the bridge flow, new views and logic. Doable but will cost time.

The second would be a small change on the UI.

But either should be thoroughly tested E2E before launch.

@Brechtpd
Copy link
Contributor

Brechtpd commented May 5, 2024

I think using PBS is still greatly preferred because it's the only mechanism that allows 100% protection of the relayer (currently the dest owner can still create difficulties). And because the proposer also requires hooking into PBS, I think having the relayer do the same shouldn't be much difference (just connect to different RPC server, but unsure how L1 fee payment would have to be changed there if at all without looking into the specifics).

However, unlike proposing, bridge transactions also need to be processed on L2 where this type of functionality will highly likely not be available for some time (though a grant was given to make something like this), so some reasonable protections for relayers there without a PBS dependency can make sense in the short term.

The approach in this PR is indeed better, but if it's short term I guess mostly depends on how much work the change in this PR brings compared to the other.

@Brechtpd
Copy link
Contributor

Brechtpd commented May 5, 2024

Should this be configurable by the user or fixed behind the scene to our relayer? If the first, this requires changes to the bridge flow, new views and logic. Doable but will cost time.

Seems fine to me if it would be fixed because it goes through our UI and our fee estimation, maybe it only needs to be a different value for other UIs?

@dantaik
Copy link
Contributor Author

dantaik commented May 6, 2024

Should this be configurable by the user or fixed behind the scene to our relayer? If the first, this requires changes to the bridge flow, new views and logic. Doable but will cost time.

The second would be a small change on the UI.

But either should be thoroughly tested E2E before launch.

It should be set by the UI/relayer directly without the user's awareness. If someone set up another dapp to help users send/process messages, that app can set the processor to another value.

@dantaik
Copy link
Contributor Author

dantaik commented May 6, 2024

I think using PBS is still greatly preferred because it's the only mechanism that allows 100% protection of the relayer (currently the dest owner can still create difficulties). And because the proposer also requires hooking into PBS, I think having the relayer do the same shouldn't be much difference (just connect to different RPC server, but unsure how L1 fee payment would have to be changed there if at all without looking into the specifics).

However, unlike proposing, bridge transactions also need to be processed on L2 where this type of functionality will highly likely not be available for some time (though a grant was given to make something like this), so some reasonable protections for relayers there without a PBS dependency can make sense in the short term.

The approach in this PR is indeed better, but if it's short term I guess mostly depends on how much work the change in this PR brings compared to the other.

In terms of implementation, I think this should be easy.

I think even with PBS, we may still want this feature so that even if someone can get transaction inclusion promise from PBS he/she cannot steal messages that sets processor to another address.

@dantaik dantaik changed the base branch from main to additional_bridge_tests May 6, 2024 03:30
Base automatically changed from additional_bridge_tests to rate_limiter May 6, 2024 08:00
Base automatically changed from rate_limiter to main May 6, 2024 08:26
@dantaik dantaik enabled auto-merge May 6, 2024 14:47
@dantaik dantaik disabled auto-merge May 6, 2024 14:48
@dantaik
Copy link
Contributor Author

dantaik commented May 7, 2024

Guys, shall we merge this PR please? Need a quick decision here.

@cyberhorsey
Copy link
Contributor

Guys, shall we merge this PR please? Need a quick decision here.

Not sure if we will even need it because I'm not expecting many relayer competitions tbh, but if you are, we can merge it and I can update relayer and korbi can do UI.

@dantaik
Copy link
Contributor Author

dantaik commented May 7, 2024

Guys, shall we merge this PR please? Need a quick decision here.

Not sure if we will even need it because I'm not expecting many relayer competitions tbh, but if you are, we can merge it and I can update relayer and korbi can do UI.

Maybe we should also have a poll for this one. @Brechtpd @adaki2004 @cyberhorsey @KorbinianK please vote against/for it. 👍 or 👎 please

@dantaik dantaik closed this May 7, 2024
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.

4 participants