Skip to content

Conversation

@nicholaspai
Copy link
Member

Protects against edge cases where a message is sent multiple times to the recipient, who holds an unexpected amount of tokens.

@nicholaspai nicholaspai requested review from mrice32 and pxrl April 21, 2023 14:11
Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

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

I think this is OK and protects against the scenario identified. It would be nice to be able to support partial fills in order to allow multiple relayers to contribute to a complete fill, but I can't see a simple and fair way to make that work at this point.

// a message is sent multiple times for multiple partial fills.
require(
relayExecution.relay.amount == fillAmountPreFees || relayExecution.updatedMessage.length == 0,
"invalid partial fill message"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"invalid partial fill message"
"invalid partial fill (message)"

Copy link
Member Author

Choose a reason for hiding this comment

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

you think this improves the error message a bit? I'm not convinced but curious what you were thinking?

Copy link
Contributor

@pxrl pxrl Apr 24, 2023

Choose a reason for hiding this comment

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

you think this improves the error message a bit? I'm not convinced but curious what you were thinking?

Yeah..even though I know exactly what the issue is, I have trouble parsing "invalid partial fill message" in my head. How about this?

Suggested change
"invalid partial fill message"
"invalid message on partial fill"

Alternatively:

Suggested change
"invalid partial fill message"
"non-zero message on partial fill"

@nicholaspai nicholaspai requested a review from pxrl April 22, 2023 15:02
@nicholaspai
Copy link
Member Author

Closed in favor of more flexible PR: #279

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