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

[DO NOT MERGE] Functions 1.1 review #11252

Closed
wants to merge 8 commits into from
Closed

Conversation

RensR
Copy link
Contributor

@RensR RensR commented Nov 10, 2023

No description provided.

Copy link
Contributor

I see that you haven't updated any README files. Would it make sense to do so?

Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

}
if (destinationNoLongerExists) {
// solidity calls check that a contract actually exists at the destination, so we do the same
if (client.code.length == 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reduced stack size so forge coverage could run

@@ -377,8 +377,7 @@ contract FunctionsRouter is IFunctionsRouter, FunctionsSubscriptions, Pausable,
commitment.estimatedTotalCostJuels,
commitment.client,
commitment.adminFee,
juelsPerGas,
SafeCast.toUint96(result.gasUsed),
juelsPerGas * SafeCast.toUint96(result.gasUsed),
Copy link
Contributor

@justinkaseman justinkaseman Nov 13, 2023

Choose a reason for hiding this comment

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

Making a breaking Router change like this would mean maintaining and deploying two different Coordinators, since existing networks won't be receiving a Router update for the next scheduled deployment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah called that out in the doc, probably ok to backlog until you'd be doing a v2

@RensR RensR closed this Nov 17, 2023
@RensR RensR deleted the functions-1.1-review branch March 30, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants