Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/share ccip bridge #180
Feat/share ccip bridge #180
Changes from 7 commits
b9c207e
03a25dc
2f9b0f0
79ecc53
d28be86
0319dd7
02d08dc
e8f3534
7b51b9c
cc54cd2
e5d785c
534603b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this error, did we want to outline the amount of fees that was erroneous? Or is it just self-explanatory since they are
user input params
again?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this error only bubbles up if the user specified
maxLinkToPay
is less than the fees it would cost to send the bridge TX. So we could probably add in 2 args, the user specified max, and the actual fees being charged. However I dont think etherscan knows how to process custom errors, so this would only help people interacting with our contracts via some dev env that tells them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that Etherscan is limited in that sense. So how would this pop up on etherscan if it were to revert because of this? If nothing can be done about it so it helps users, then that is fine to leave this unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caller will have to approve
LINK
to be sent to thisDestinationMinter
, add note in the natspec?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On that note, do we want to have a
revokeApprovals
at the end of this function for LINK?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm that is a good question. The router address is verified to be safe, and these contracts will not be holding any customer funds that are LINK. So it would probably only help us if for some reason the router did not transfer the link it said it would, and the LINK contract had ERC20 breaking logic like USDT where we revert when going from non zero allowance to non zero allowance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but since we know that is not the case with the
LINK
contract, are you saying that we do not need to have arevokeApprovals
applied here?