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

Functions Router #9365

Merged
merged 40 commits into from
Jul 18, 2023
Merged

Functions Router #9365

merged 40 commits into from
Jul 18, 2023

Conversation

justinkaseman
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

2 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

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

2 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2023

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

@github-actions
Copy link
Contributor

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

Copy link
Contributor

@pinebit pinebit left a comment

Choose a reason for hiding this comment

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

Provided initial round comments.

contracts/src/v0.8/functions/dev/1_0_0/Functions.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/functions/dev/1_0_0/RouterBase.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/functions/dev/1_0_0/RouterBase.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/functions/dev/1_0_0/RouterBase.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/functions/dev/1_0_0/FunctionsRouter.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/functions/dev/1_0_0/FunctionsRouter.sol Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

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

Copy link
Contributor

@KuphJr KuphJr left a comment

Choose a reason for hiding this comment

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

These are my comments so far. I started to become confused while reviewing FunctionsSubscriptions.sol.

I still need to complete reviewing /tests/, */1_0_0/FunctionsBilling.sol, */1_0_0/FunctionsClient.sol, */1_0_0/FunctionsCoordinator.sol, */1_0_0/FunctionsRouter.sol and */1_0_0/FunctionsSubscriptions.sol

Copy link
Contributor

@KuphJr KuphJr left a comment

Choose a reason for hiding this comment

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

These are my comments so far. I started to become confused while reviewing FunctionsSubscriptions.sol.

I still need to complete reviewing /tests/, */1_0_0/FunctionsBilling.sol, */1_0_0/FunctionsClient.sol, */1_0_0/FunctionsCoordinator.sol, */1_0_0/FunctionsRouter.sol and */1_0_0/FunctionsSubscriptions.sol

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

Location codeLocation;
Location secretsLocation; // Only Remote secrets are supported
CodeLanguage language;
string source; // Source code for Location.Inline, url for Location.Remote or slot decimal number for Location.DONHosted
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be my bad, but we shall repeat the logic for source regarding DONHosted variant.
Maybe we can even add the same helper addDONHostedSource()

Location secretsLocation; // Only Remote secrets are supported
CodeLanguage language;
string source; // Source code for Location.Inline, url for Location.Remote or slot decimal number for Location.DONHosted
bytes encryptedSecretsReference; // Encrypted urls for Location.Remote or CBOR encoded slotid+version for Location.DONHosted, use addDONHostedSecrets()
Copy link
Contributor

Choose a reason for hiding this comment

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

@KuphJr since DONHosted option does not use encryption, shall we rename this field to be just secretsReference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the DONHosted secrets are technically encrypted, so I think the name still works?

/**
* @notice Uses current price feed data to estimate a cost
*/
function _calculateCostEstimate(
Copy link
Contributor

Choose a reason for hiding this comment

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

What are we gonna do about the problem of an increase in gas price between time of request and time of response? Should we add an overestimate percentage for gas price or something? This could be a config variable.

pinebit
pinebit previously approved these changes Jul 18, 2023
Copy link
Contributor

@pinebit pinebit left a comment

Choose a reason for hiding this comment

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

This is to indicate I am good with the changes.
(except the DONHosted thing for secrets/source)
After you address that, treat my approval is valid.

KuphJr
KuphJr previously approved these changes Jul 18, 2023
@justinkaseman justinkaseman dismissed stale reviews from KuphJr and pinebit via 636143e July 18, 2023 17:54
@KuphJr KuphJr self-requested a review July 18, 2023 17:56
KuphJr
KuphJr previously approved these changes Jul 18, 2023
@justinkaseman justinkaseman added this pull request to the merge queue Jul 18, 2023
@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 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023
@justinkaseman justinkaseman added this pull request to the merge queue Jul 18, 2023
Merged via the queue into develop with commit 6252423 Jul 18, 2023
@justinkaseman justinkaseman deleted the jk/functions-router-simple branch July 18, 2023 22:37
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.

5 participants