remove oracle withdraw and allow contract owner to withdraw #11551

jinhoonbang · 2023-12-12T22:33:19Z

remove oracle withdraw and allow contract owner to withdraw
open question. should there be an additional identity who can withdraw?
- will increase contract size

Testing

On-chain testing

$ forge test -vvv --match-path test/v0.8/foundry/vrf/VRFV2PlusSubscriptionAPI.t.sol
...
Test result: ok. 32 passed; 0 failed; 0 skipped; finished in 7.41ms
Ran 1 test suites: 32 tests passed, 0 failed, 0 skipped (32 total tests)

Off-chain integration testing

$ cd integration-tests
$ make build_docker_image image=chainlink tag=local_dev
$ TEST_LOG_LEVEL=debug \
SELECTED_NETWORKS=SIMULATED \
CHAINLINK_IMAGE=chainlink \
CHAINLINK_VERSION=local_dev \
go test -v -count=1 -timeout 15m -run "^TestVRFv2Plus$" ./smoke

github-actions · 2023-12-12T22:33:37Z

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

contracts/src/v0.8/vrf/dev/SubscriptionAPI.sol

contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol

core/services/vrf/v2/integration_helpers_test.go

integration-tests/smoke/vrfv2plus_test.go

github-actions · 2023-12-18T11:15:34Z

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

…he withdrawable amount

jinhoonbang · 2023-12-20T21:57:11Z

contracts/scripts/native_solc_compile_all_vrfv2plus

@@ -0,0 +1,60 @@
+#!/usr/bin/env bash


is this needed? this is already in https://github.com/smartcontractkit/chainlink/blob/develop/contracts/scripts/native_solc_compile_all_vrf

the naming convention here is defined here . I don't think maintaining a separate foundry profile (e.g. vrfv2plus) is needed

ibrajer · 2023-12-21T10:30:48Z

contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol

   * @param publicProvingKey key that oracle can use to submit vrf fulfillments
   */
-  function registerProvingKey(address oracle, uint256[2] calldata publicProvingKey) external onlyOwner {
+  function registerProvingKey(uint256[2] calldata publicProvingKey) external onlyOwner {


@jinhoonbang @kidambisrinivas FYI this is a breaking change for Gauntlet commands (both for register and deregister proving key). When I use this branch, unit tests on the Gauntlet repo for proving key registration and deregistration are failing. I will fix those commands in the same PR where I would be introducing new withdrawal functions. Then, we would merge it on the Gauntlet develop branch only when this PR is merged (and after I double-check everything once again).

…contract-owner-to-withdraw

dkneisly · 2023-12-22T15:35:22Z

core/services/vrf/v2/coordinator_v2x_interface.go

@@ -36,9 +36,10 @@ type CoordinatorV2_X interface {
 	GetConfig(opts *bind.CallOpts) (Config, error)
 	ParseLog(log types.Log) (generated.AbigenLog, error)
 	OracleWithdraw(opts *bind.TransactOpts, recipient common.Address, amount *big.Int) (*types.Transaction, error)
+	Withdraw(opts *bind.TransactOpts, recipient common.Address) (*types.Transaction, error)


Do we need a second method here for withdrawing native?

Yeah we can add it for completeness. It seems that Withdraw() isn't used anywhere currently either

In terms of test coverage, withdrawNative is tested in integration tests which uses a different interface from this one.

So this interface is not tested or is just the Withdraw() method not used/tested? And why do integration tests use a different interface?

The interface is used and tested but seems that a few methods (withdraw() and withdrawNative()) are unused.

The integration tests use different interface but I'm not sure if we have a good reason for it. I think we have room for some refactoring in integration tests to either create a new shared interface for v2 and v2plus or use the existing one above.

cl-sonarqube-production · 2023-12-22T17:05:15Z

SonarQube Quality Gate

33.3% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

ibrajer

I'm approving the PR to unblock you this week - but please take it with a grain of salt. Also, since this is impacting only v2.5 it should not break anything and would probably be okay to patch things next week (if needed).

remove oracle withdraw and allow contract owner to withdraw

11ff2f5

jinhoonbang requested a review from a team as a code owner December 12, 2023 22:33

jinhoonbang requested review from kidambisrinivas, dkneisly, ibrajer and iljapavlovs December 12, 2023 22:34

kidambisrinivas reviewed Dec 13, 2023

View reviewed changes

contracts/src/v0.8/vrf/dev/SubscriptionAPI.sol Outdated Show resolved Hide resolved

contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol Show resolved Hide resolved

jinhoonbang added 2 commits December 13, 2023 11:59

offchain changes

e456aac

fix integration tests

b3c1d09

jinhoonbang requested a review from a team as a code owner December 13, 2023 21:20

fix integration tests

54a523e

jinhoonbang requested a review from kidambisrinivas December 13, 2023 22:03

dkneisly reviewed Dec 14, 2023

View reviewed changes

core/services/vrf/v2/integration_helpers_test.go Outdated Show resolved Hide resolved

iljapavlovs reviewed Dec 14, 2023

View reviewed changes

integration-tests/smoke/vrfv2plus_test.go Outdated Show resolved Hide resolved

Remove amount arg in withdraw and withdraw all the withdrawable amount

94b1d53

Off-chain changes to remove amount arg in withdraw and withdraw all t…

6b4b478

…he withdrawable amount

jinhoonbang commented Dec 20, 2023

View reviewed changes

ibrajer reviewed Dec 21, 2023

View reviewed changes

jinhoonbang added 3 commits December 21, 2023 14:32

address comments

279fec7

Merge branch 'develop' into VRF-797-remove-oracle-withdraw-and-allow-…

3a7a000

…contract-owner-to-withdraw

fix lint issue and small refactor in vrf integration tests

14f4877

jinhoonbang requested review from ibrajer, dkneisly and iljapavlovs December 21, 2023 23:00

dkneisly reviewed Dec 22, 2023

View reviewed changes

address comment

a319513

jinhoonbang requested a review from dkneisly December 22, 2023 17:38

ibrajer approved these changes Jan 2, 2024

View reviewed changes

jinhoonbang added this pull request to the merge queue Jan 2, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 2, 2024

jinhoonbang added this pull request to the merge queue Jan 2, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 2, 2024

jinhoonbang added this pull request to the merge queue Jan 2, 2024

Merged via the queue into develop with commit c8eaac7 Jan 2, 2024
90 of 91 checks passed

jinhoonbang deleted the VRF-797-remove-oracle-withdraw-and-allow-contract-owner-to-withdraw branch January 2, 2024 19:52

This was referenced Feb 22, 2024

release/2.9.0 -> master #12148

Closed

Chore/release 2.9.0 -> master #12190

Closed

snehaagni mentioned this pull request Mar 7, 2024

chore/release 2.9.1 to master #12346

Closed

This was referenced Mar 21, 2024

chore/release 2.9.1 to master take2 #12530

Closed

chore/release 2.9.1 to master take3 #12551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove oracle withdraw and allow contract owner to withdraw #11551

remove oracle withdraw and allow contract owner to withdraw #11551

jinhoonbang commented Dec 12, 2023 •

edited by kidambisrinivas

Loading

github-actions bot commented Dec 12, 2023

github-actions bot commented Dec 18, 2023

jinhoonbang Dec 20, 2023

jinhoonbang Dec 20, 2023

ibrajer Dec 21, 2023

dkneisly Dec 22, 2023

jinhoonbang Dec 22, 2023 •

edited

Loading

jinhoonbang Dec 22, 2023

ibrajer Dec 22, 2023

jinhoonbang Dec 28, 2023 •

edited

Loading

cl-sonarqube-production bot commented Dec 22, 2023

ibrajer left a comment

remove oracle withdraw and allow contract owner to withdraw #11551

remove oracle withdraw and allow contract owner to withdraw #11551

Conversation

jinhoonbang commented Dec 12, 2023 • edited by kidambisrinivas Loading

Testing

On-chain testing

Off-chain integration testing

github-actions bot commented Dec 12, 2023

github-actions bot commented Dec 18, 2023

jinhoonbang Dec 20, 2023

Choose a reason for hiding this comment

jinhoonbang Dec 20, 2023

Choose a reason for hiding this comment

ibrajer Dec 21, 2023

Choose a reason for hiding this comment

dkneisly Dec 22, 2023

Choose a reason for hiding this comment

jinhoonbang Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

jinhoonbang Dec 22, 2023

Choose a reason for hiding this comment

ibrajer Dec 22, 2023

Choose a reason for hiding this comment

jinhoonbang Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Dec 22, 2023

ibrajer left a comment

Choose a reason for hiding this comment

jinhoonbang commented Dec 12, 2023 •

edited by kidambisrinivas

Loading

jinhoonbang Dec 22, 2023 •

edited

Loading

jinhoonbang Dec 28, 2023 •

edited

Loading