Skip to content

Conversation

@nicholaspai
Copy link
Member

No description provided.

@nicholaspai nicholaspai requested a review from mrice32 March 3, 2022 21:29
"setEnableRoute(address,uint256,bool)",
originToken,
destinationChainId,
(destinationToken != address(0)) // We assume that setting the destination token to 0x0 for any route
Copy link
Member

Choose a reason for hiding this comment

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

the other option would be to add a bool to the props and let the caller pass it in. either is ok I think but what you have here does make some implementations impossible (if the target chain actually uses destinationToken of 0x0, which should never happen)

Copy link
Member Author

Choose a reason for hiding this comment

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

let's add a bool to the function call params then as the gas increase is not important for a function that shoudn't be called that frequently

Copy link
Contributor

Choose a reason for hiding this comment

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

If we did it with a bool, we'd need additional branching at the top of this method.

I do think using 0x0 is a bit more subtle, but the implementation might be a little cleaner this way.

I think either way makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think additional branching is fine for functions where we are not as gas cost sensitive like this one

"setEnableRoute(address,uint256,bool)",
originToken,
destinationChainId,
(destinationToken != address(0)) // We assume that setting the destination token to 0x0 for any route
Copy link
Contributor

Choose a reason for hiding this comment

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

If we did it with a bool, we'd need additional branching at the top of this method.

I do think using 0x0 is a bit more subtle, but the implementation might be a little cleaner this way.

I think either way makes sense.

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