fix: ABCI Removal Fix of Currency Pairs

[davidterpay]

Let's walk through a few scenarios:

Example 1

Height = H. With no removals in H.
N currency pairs in state.

• Extend vote with N CPs.
• Verify Vote with N CPs.
• H is committed.
• PrepareProposal checking that each VE has N CPs maximally in H + 1.
• ProcessProposal checking that each VE has N CPs maximally in H + 1.

Example 2

Height = H. With M removals in H.
N currency pairs in state.

• Extend Vote with N CPs.
• Verify Vote with N CPs.
• H is committed. This updates the num removed to M. This updates current amount stored to Z = N - M.
• PrepareProposal checking each VE has Z + M CPs maximally in H+1.
• ProcessProposal checking each VE has Z + M CPs maximally in H+1.

Example 3

Height = H. With removals in H+1.
N currency pairs in state.

• Extend vote with N CPs.
• Verify Vote with N CPs.
• H is committed.
• PrepareProposal checking that each VE has N CPs maximally in H+1.
• ProcessProposal checking that each VE has N CPs maximally in H+1.
• Extend Vote / Verify Vote logic is repeated in example 2.

[davidterpay]

We inherit the default one so we don't need this since the check is the same.

[codecov]

Codecov Report

Attention: Patch coverage is 63.63636% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 56.82%. Comparing base (043ce53) to head (eb68c2c).

Files	Patch %	Lines
abci/strategies/currencypair/default.go	55.55%	2 Missing and 2 partials ⚠️
x/oracle/keeper/keeper.go	69.23%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #300      +/-   ##
==========================================
+ Coverage   56.74%   56.82%   +0.07%     
==========================================
  Files         227      227              
  Lines       10756    10761       +5     
==========================================
+ Hits         6104     6115      +11     
+ Misses       4139     4134       -5     
+ Partials      513      512       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aljo242]

Suggested change

	removed, err := s.oracleKeeper.GetNumRemovedCurrencyPairs(ctx)
	removed, err := s.oracleKeeper.GetNumRemovedCurrencyPairsInBlock(ctx)

I think let's be as pedantic as possible here since this is tricky enough to reason about

[nivasan1]

Instead of implicitly casing on the exec-mode, I'd ideally like to expose to the caller whether they want the value of cps in state before the block was executed (specifically in Prepare / Process), or whether they want the # of cps in state (Verify)

[nivasan1]

This function has two diff behaviors AFAICT:

1. Get the # of cps in state before the block was executed (+ all cps added in the last block executed)
2. Get the current # of cps in state

[davidterpay]

that would a breaking change with the current dYdX integration so im in favor of keeping it as is.

[nivasan1]

This alr is a breaking change, no? Only the GetPrevBlockCPCounter is implemented in dydx

[aljo242]

goated

[aljo242]

this is just for knowledge sake - if this does not return an error if the key does not exist, what are the cases where this does error?

[nivasan1]

So in this case, we're leaving (numRemoved + numAddedCPs) * 33bz bytes unaccounted for (if we're j checking the size of the vote-extension. What is the advantage of doing this, in comparison to checking per ID whether the cp reported for either existed in state or was removed (we originally proposed this)?

[nivasan1]

left comment

[aljo242]

So in this case, we're leaving (numRemoved + numAddedCPs) * 33bz bytes unaccounted for (if we're j checking the size of the vote-extension. What is the advantage of doing this, in comparison to checking per ID whether the cp reported for either existed in state or was removed (we originally proposed this)?

The main one in my mind is that we are doing significantly less state reads this way. O(1) v O(n)

[davidterpay]

@nivasan1 we originally had alignment that we would check only the size, you proposed the other idea but we decided to go with the simpler approach. I suggest we leave this as is and ask ottersec to take a deeper dive into this. More likely than not I can see the possibility of us moving into checking the IDs.

[aljo242]

Approving for now but @davidterpay can you create an issue to clean this up / rename functions / doc this?

[nivasan1]

lgtm