redo amend liquidity provision, with a cancel and replace approach

[jeremyletang]

This update the lp amendment to use an cancel and replace approach.
This is not ready for merging, I'll be adding a bunch of tests to cover amending of submissions.

close #3065
close #3064

[jeremyletang]

staticcheck provides a way to ignore warnings: https://staticcheck.io/docs/#ignoring-problems

Yes I'm aware, I just don't like the idea of adding these comments in the code so they get slightly deprecated when not valid anymore like the one I removed as well.

I just ended up changing the implementation, I suppose I would be using a real panic it would have pass though.

[qustavo]

Normally staticcheck should warn you about unmatched ignore directives:

execution/market.go:2613:53: this linter directive didn't match anything; should it be removed? (staticcheck)

[jeremyletang]

Normally staticcheck should warn you about unmatched ignore directives:

execution/market.go:2613:53: this linter directive didn't match anything; should it be removed? (staticcheck)

Yes it does, but it’s been there forever and no one took care of it.

[qustavo]

Might be we should try to fix it properly then, I'm running make statickcheck and the warning it's making it fail, but still the CI is passing, any idea why this might be happening @ashleyvega ?

[jeremyletang]

Might be we should try to fix it properly then, I'm running make statickcheck and the warning it's making it fail, but still the CI is passing, any idea why this might be happening @ashleyvega ?

➜  vega git:(feature/fix-amend-liquidity-provision) ✗ staticcheck ./execution
execution/market.go:32:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (SA1019)

It appears to work on this branch though?

[qustavo]

Might be we should try to fix it properly then, I'm running make statickcheck and the warning it's making it fail, but still the CI is passing, any idea why this might be happening @ashleyvega ?

➜  vega git:(feature/fix-amend-liquidity-provision) ✗ staticcheck ./execution
execution/market.go:32:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (SA1019)

It appears to work on this branch though?

that's because you are running staticcheck with all checks, we ignore some directives, see https://github.com/vegaprotocol/vega/blob/develop/script/build.sh#L298

[jeremyletang]

Might be we should try to fix it properly then, I'm running make statickcheck and the warning it's making it fail, but still the CI is passing, any idea why this might be happening @ashleyvega ?

➜  vega git:(feature/fix-amend-liquidity-provision) ✗ staticcheck ./execution
execution/market.go:32:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (SA1019)

It appears to work on this branch though?

that's because you are running staticcheck with all checks, we ignore some directives, see https://github.com/vegaprotocol/vega/blob/develop/script/build.sh#L298

Wait there's some logic I don't get, because I run staticcheck with all the checks, which should make it stricter ultimately, it makes it work on my branch?

I would think that the issue comes not from me then?

[qustavo]

I think so too, looks like we are ignoring staticcheck output on the ci, but I can't tell 100% sure

[ashleyvega]

CI watches for command exit codes. The command we run exits with code zero, despite complaining about execution/market.go:2613.

vega/script/build.sh

Line 298 in 20ee7fb

staticcheck -checks 'all,-SA1019,-ST1000,-ST1021' ./...

[ashleyvega]

• staticcheck -checks all ./... - 2438 warnings, exit code 1
• staticcheck -checks all ./... | grep -vF '.pb.go' - 342 warnings, exit code 1
• staticcheck -checks 'all,-SA1019,-ST1000,-ST1021' ./... - 1 warning, exit code 0

[qustavo]

Thanks for checking @ashleyvega now it gets weird, look at my output:

staticcheck -checks 'all,-SA1019,-ST1000,-ST1021' ./...
  execution/market.go:2613:53: this linter directive didn't match anything; should it be removed? (staticcheck)
  Run 'staticcheck -explain <check>' or visit https://staticcheck.io/docs/checks for documentation on checks.

echo $?
1

[qustavo]

➜  ~ staticcheck -version
staticcheck 2020.2 (v0.1.0)

[ashleyvega]

I have staticcheck 2020.1.4 locally, as does CI (ref).

Nothing weird here.

[qustavo]

I think that upgrading version will mark the check failed on that warning

[ashleyvega]

Perhaps a PR that bumps the staticcheck version in gettools.sh and fixes execution/market.go would be welcome. (And then a similar update in devops-infra.)

[qustavo]

Perhaps a PR that bumps the staticcheck version in gettools.sh and fixes execution/market.go would be welcome. (And then a similar update in devops-infra.)

https://github.com/vegaprotocol/devops-infra/pull/412

[qustavo]

staticcheck provides a way to ignore warnings: https://staticcheck.io/docs/#ignoring-problems

Yes I'm aware, I just don't like the idea of adding these comments in the code so they get slightly deprecated when not valid anymore like the one I removed as well.

Finally! when the new staticcheck version is in the CI you can be safe that deprecated messages will make the ci fail.