Update Simulated Backend Client #10403

EasterTheBunny · 2023-08-30T15:13:22Z

This commit aims to improve the simulated backend client by removing the block limit where only the latest block can be provided to CallContext and handling input args more gracefully.

CallContext will now only validate the block number passed in args
CallContext will NOT restrict calls to current block, but will pass nil to backend
from in call args is no longer required and will default to 0x
from and to accept common.Address, *big.Int and string types

Included is a move to Go standard error handling consistent with v1.20+ and error wrapping.

This commit aims to improve the simulated backend client by removing the block limit where only the latest block can be provided to `CallContext` and handling input args more gracefully. - `CallContext` will now only validate the block number passed in args - `CallContext` will NOT restrict calls to current block, but will pass nil to backend - `from` in call args is no longer required and will default to `0x` - `from` and `to` accept `common.Address`, `*big.Int` and `string` types Included is a move to Go standard error handling consistent with v1.20+ and error wrapping.

github-actions · 2023-08-30T15:13:44Z

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

core/chains/evm/client/simulated_backend_client.go

RyanRHall · 2023-08-30T16:24:42Z

core/chains/evm/client/simulated_backend_client.go

+	default:
+		return common.HexToAddress("0x")


maybe we should error instead of defaulting to 0x000.. if the arg is an unsupported type?

That would be less silent. I like it.

RyanRHall · 2023-08-30T16:26:38Z

core/chains/evm/client/simulated_backend_client.go

@@ -155,26 +184,26 @@ func init() {
 	var err error
 	balanceOfABI, err = abi.JSON(strings.NewReader(balanceOfABIString))
 	if err != nil {
-		panic(errors.Wrapf(err, "while parsing erc20ABI"))
+		panic(fmt.Errorf("%w: while parsing erc20ABI", err))


why get rid of errors.Wrap() ? We use it all over core and it kind of removes the debate over "how to wrap your errors"

We use it all over core because it predates the latest error wrapping features of Go. This is just making a small change in a single file to bring the code up to date with latest Go standards.

ahh I didn't realize that. Thanks for the explanation :)

I did research this one a bit just to be sure so I'll add a bit more info here for anyone else interested in why this change.

github.com/pkg/errors is in maintenance mode because of the latest Go standard of wrapping errors

using errors.Is from the above package simply wraps the Go standard errors.Is so no threat from not using Wrap

go-ethereum returns errors in the standard format, so continuing that pattern in our own simulated client makes the most sense

RyanRHall · 2023-08-30T16:30:56Z

core/chains/evm/client/simulated_backend_client.go

+	} else {
+		ca.From = common.HexToAddress("0x")
+	}
+
 	return &ca, blockNumber, nil


what happens now if we pass an old block number? Does the backend error or does it just use latest anyway?

It will just use latest by default. This change makes the behavior of CallContext and BatchCallContext match regarding the block number passed to the simulated backend.

jmank88 · 2023-09-01T16:13:49Z

core/chains/evm/client/simulated_backend_client.go

+/*
+The simulated backend cannot access old blocks and will return an error if
+anything other than `latest`, `nil`, or the latest block are passed to
+`CallContract`.
+
+The simulated client avoids the old block error from the simulated backend by
+passing `nil` to `CallContract` when calling `CallContext` or `BatchCallContext`
+and will not return an error when an old block is used.
+*/


Does godoc pick this up as a package level comment? Is there a particular type we could associate it with instead?

I added it to the top so that the behavior of two functions is well explained and visible. I could add the explanation to each of the functions in question CallContext and BatchCallContext as well to ensure the explanation is visible to IDE tools as well.

There isn't a doc.go file in that package, but I can create one and move this comment there too.

Hmmm I thought a doc.go was just a convention for humans to find it in source. I don't think godoc and pkg.go.dev care about the file boundaries though 🤔
I usually run godoc -http=:6060 locally to try it out

Yeah it just puts all the detached floating file comments like this at the top in the overview:

Haha! Therefore it's duplicated. The preference would be to put it in doc.go then?

Yeah, unless there is a specific identifier to associate it with instead.

core/chains/evm/client/simulated_backend_client.go

prashantkumar1982 · 2023-09-04T06:42:46Z

core/chains/evm/client/simulated_backend_client.go

+func interfaceToAddress(value interface{}) (common.Address, error) {
+	switch v := value.(type) {
+	case common.Address:
+		return v


missing return argument.
return v, nil

prashantkumar1982 · 2023-09-04T06:43:43Z

core/chains/evm/client/simulated_backend_client.go

+
+		ca.From = addr
+	} else {
+		ca.From = common.HexToAddress("0x")


This seems weird. Why don't we just err here?
FromAddress being 0x will anyways not work i later part of code.

I have it like this because other parts of the code don't provide a 'From' value as I suppose the production client doesn't require it. I tried to model the simulated client to follow the same behavior.

prashantkumar1982 · 2023-09-04T06:45:28Z

core/chains/evm/client/simulated_backend_client.go

 	}
+
+	// to and from need to map to a common.Address but could come in as a string


nit: comment unaligned.
Maybe move the toAddr and fromAddr parsing before setting CallArgs, and put this comment there.

prashantkumar1982 · 2023-09-04T06:46:41Z

core/chains/evm/client/simulated_backend_client.go

 // CallContext mocks the ethereum client RPC calls used by chainlink, copying the
 // return value into result.
+// The simulated backend cannot access old blocks and will return an error if


Looks like the 1st paragraph here needs to be deleted?
The next para seems the right description as per this change

prashantkumar1982 · 2023-09-04T06:49:15Z

core/chains/evm/client/simulated_backend_client.go

@@ -459,6 +512,12 @@ func (c *SimulatedBackendClient) SuggestGasPrice(ctx context.Context) (*big.Int,
 }

 // BatchCallContext makes a batch rpc call.
+// The simulated backend cannot access old blocks and will return an error if
+// anything other than `latest`, `nil`, or the latest block are passed to


delete 1st para here, seems outdated.

cl-sonarqube-production · 2023-09-06T21:25:47Z

SonarQube Quality Gate

3.3% Coverage on New Code (is less than 80%)

See analysis details on SonarQube

EasterTheBunny requested review from samsondav and prashantkumar1982 as code owners August 30, 2023 15:13

amirylm reviewed Aug 30, 2023

View reviewed changes

core/chains/evm/client/simulated_backend_client.go Outdated Show resolved Hide resolved

remove commented code

9cdeab1

RyanRHall reviewed Aug 30, 2023

View reviewed changes

add comments at top of file and panic on unrecognized value type

f2e0675

jmank88 reviewed Sep 1, 2023

View reviewed changes

core/chains/evm/client/simulated_backend_client.go Outdated Show resolved Hide resolved

updated error handling and comments

a5b4d8b

prashantkumar1982 reviewed Sep 4, 2023

View reviewed changes

address comments

1d2ad05

jmank88 approved these changes Sep 5, 2023

View reviewed changes

prashantkumar1982 approved these changes Sep 6, 2023

View reviewed changes

Merge branch 'develop' into AUTO-4826/update-sim-backend

4d2de62

EasterTheBunny enabled auto-merge September 6, 2023 15:41

EasterTheBunny added this pull request to the merge queue Sep 6, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 6, 2023

Merge branch 'develop' into AUTO-4826/update-sim-backend

6c120a6

EasterTheBunny enabled auto-merge September 6, 2023 22:48

EasterTheBunny added this pull request to the merge queue Sep 6, 2023

Merged via the queue into develop with commit 3dfb527 Sep 6, 2023

EasterTheBunny deleted the AUTO-4826/update-sim-backend branch September 6, 2023 23:13

snehaagni mentioned this pull request Oct 18, 2023

release/2.6.0 -> master #10997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Simulated Backend Client #10403

Update Simulated Backend Client #10403

EasterTheBunny commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

RyanRHall Aug 30, 2023

EasterTheBunny Aug 30, 2023

RyanRHall Aug 30, 2023

EasterTheBunny Aug 30, 2023

RyanRHall Aug 30, 2023

EasterTheBunny Aug 31, 2023 •

edited

Loading

RyanRHall Aug 30, 2023

EasterTheBunny Aug 30, 2023

jmank88 Sep 1, 2023

EasterTheBunny Sep 1, 2023 •

edited

Loading

EasterTheBunny Sep 1, 2023 •

edited

Loading

jmank88 Sep 1, 2023

jmank88 Sep 1, 2023

EasterTheBunny Sep 5, 2023

jmank88 Sep 5, 2023

prashantkumar1982 Sep 4, 2023

EasterTheBunny Sep 5, 2023

prashantkumar1982 Sep 4, 2023

EasterTheBunny Sep 5, 2023

prashantkumar1982 Sep 4, 2023

prashantkumar1982 Sep 4, 2023

prashantkumar1982 Sep 4, 2023

cl-sonarqube-production bot commented Sep 6, 2023

		}

		// to and from need to map to a common.Address but could come in as a string

Update Simulated Backend Client #10403

Update Simulated Backend Client #10403

Conversation

EasterTheBunny commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EasterTheBunny Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EasterTheBunny Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

EasterTheBunny Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Sep 6, 2023

EasterTheBunny Aug 31, 2023 •

edited

Loading

EasterTheBunny Sep 1, 2023 •

edited

Loading

EasterTheBunny Sep 1, 2023 •

edited

Loading