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

feat(oracle): Add new DatedExchangeRate query with block timestamp and block height #2097

Merged
merged 7 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ Zenith](https://code4rena.com/zenith) Audit, running from 2024-10-07 until
period. This section describes code changes that occured after that audit in
preparation for a second audit starting in November 2024.

- [#2074](https://github.com/NibiruChain/nibiru/pull/2074) - fix(evm-keeper): better utilize ERC20 metadata during FunToken creation. The bank metadata for a new FunToken mapping ties a connection between the Bank Coin's `DenomUnit` and the ERC20 contract metadata like the name, decimals, and symbol. This change brings parity between EVM wallets, such as MetaMask, and Interchain wallets like Keplr and Leap.
- [#2074](https://github.com/NibiruChain/nibiru/pull/2074) - fix(evm-keeper): better utilize ERC20 metadata during FunToken creation. The bank metadata for a new FunToken mapping ties a connection between the Bank Coin's `DenomUnit` and the ERC20 contract metadata like the name, decimals, and symbol. This change brings parity between EVM wallets, such as MetaMask, and Interchain wallets like Keplr and Leap.
- [#2076](https://github.com/NibiruChain/nibiru/pull/2076) - fix(evm-gas-fees):
Use effective gas price in RefundGas and make sure that units are properly
reflected on all occurences of "base fee" in the codebase. This fixes [#2059](https://github.com/NibiruChain/nibiru/issues/2059)
and the [related comments from @Unique-Divine and @berndartmueller](https://github.com/NibiruChain/nibiru/issues/2059#issuecomment-2408625724).
Use effective gas price in RefundGas and make sure that units are properly
reflected on all occurences of "base fee" in the codebase. This fixes [#2059](https://github.com/NibiruChain/nibiru/issues/2059)
and the [related comments from @Unique-Divine and @berndartmueller](https://github.com/NibiruChain/nibiru/issues/2059#issuecomment-2408625724).
- [#2084](https://github.com/NibiruChain/nibiru/pull/2084) - feat(evm-forge): foundry support and template for Nibiru EVM develoment
- [#2086](https://github.com/NibiruChain/nibiru/pull/2086) - fix(evm-precomples):
Fix state consistency in precompile execution by ensuring proper journaling of
Expand All @@ -68,6 +68,10 @@ consistent setup and dynamic gas calculations, addressing the following tickets.
- <https://github.com/code-423n4/2024-10-nibiru-zenith/issues/47>
- [#2088](https://github.com/NibiruChain/nibiru/pull/2088) - refactor(evm): remove outdated comment and improper error message text
- [#2089](https://github.com/NibiruChain/nibiru/pull/2089) - better handling of gas consumption within erc20 contract execution
- [#2090](https://github.com/NibiruChain/nibiru/pull/2090) - fix(evm): Account
for (1) ERC20 transfers with tokens that return false success values instead of
throwing an error and (2) ERC20 transfers with other operations that don't bring
about the expected resulting balance for the transfer recipient.
- [#2091](https://github.com/NibiruChain/nibiru/pull/2091) - feat(evm): add fun token creation fee validation
- [#2094](https://github.com/NibiruChain/nibiru/pull/2094) - fix(evm): Following
from the changs in #2086, this pull request implements a new `JournalChange`
Expand All @@ -78,12 +82,10 @@ non-EVM and EVM state will be in sync even if there are complex, multi-step
Ethereum transactions, such as in the case of an EthereumTx that influences the
`StateDB`, then calls a precompile that also changes non-EVM state, and then EVM
reverts inside of a try-catch.
- [#2098](https://github.com/NibiruChain/nibiru/pull/2098) - test(evm): statedb tests for race conditions within funtoken precompile
- [#2090](https://github.com/NibiruChain/nibiru/pull/2090) - fix(evm): Account
for (1) ERC20 transfers with tokens that return false success values instead of
throwing an error and (2) ERC20 transfers with other operations that don't bring
about the expected resulting balance for the transfer recipient.
- [#2092](https://github.com/NibiruChain/nibiru/pull/2092) - feat(evm): add validation for wasm multi message execution
- [#2097](https://github.com/NibiruChain/nibiru/pull/2097) - feat(evm): Add new query to get dated price from the oracle precompile
- [#2098](https://github.com/NibiruChain/nibiru/pull/2098) - test(evm): statedb tests for race conditions within funtoken precompile
- [#2100](https://github.com/NibiruChain/nibiru/pull/2100) - refactor: cleanup statedb and precompile sections

#### Nibiru EVM | Before Audit 1 - 2024-10-18

Expand Down Expand Up @@ -163,7 +165,6 @@ about the expected resulting balance for the transfer recipient.
- [#2060](https://github.com/NibiruChain/nibiru/pull/2060) - fix(evm-precompiles): add assertNumArgs validation
- [#2056](https://github.com/NibiruChain/nibiru/pull/2056) - feat(evm): add oracle precompile
- [#2065](https://github.com/NibiruChain/nibiru/pull/2065) - refactor(evm)!: Refactor out dead code from the evm.Params
- [#2100](https://github.com/NibiruChain/nibiru/pull/2100) - refactor: cleanup statedb and precompile sections

### State Machine Breaking (Other)

Expand Down
1 change: 1 addition & 0 deletions app/wasmext/stargate_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ func WasmAcceptedStargateQueries() wasmkeeper.AcceptedStargateQueries {

// nibiru oracle
"/nibiru.oracle.v1.Query/ExchangeRate": new(oracle.QueryExchangeRateResponse),
"/nibiru.oracle.v1.Query/DatedExchangeRate": new(oracle.QueryDatedExchangeRateResponse),
"/nibiru.oracle.v1.Query/ExchangeRateTwap": new(oracle.QueryExchangeRateResponse),
"/nibiru.oracle.v1.Query/ExchangeRates": new(oracle.QueryExchangeRatesResponse),
"/nibiru.oracle.v1.Query/Actives": new(oracle.QueryActivesResponse),
Expand Down
26 changes: 25 additions & 1 deletion proto/nibiru/oracle/v1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ service Query {
option (google.api.http).get = "/nibiru/oracle/v1beta1/exchange_rate_twap";
}

// DatedExchangeRate returns latest price of a pair
rpc DatedExchangeRate(QueryExchangeRateRequest)
returns (QueryDatedExchangeRateResponse) {
option (google.api.http).get = "/nibiru/oracle/v1beta1/dated_exchange_rate";
}

// ExchangeRates returns exchange rates of all pairs
rpc ExchangeRates(QueryExchangeRatesRequest)
returns (QueryExchangeRatesResponse) {
Expand Down Expand Up @@ -112,7 +118,25 @@ message QueryExchangeRateResponse {

// QueryExchangeRatesRequest is the request type for the Query/ExchangeRates RPC
// method.
message QueryExchangeRatesRequest {}
message QueryExchangeRatesRequest {
}

// QueryDatedExchangeRateResponse is the request type for the
// Query/DatedExchangeRate RPC method.
message QueryDatedExchangeRateResponse {
string price = 1 [
(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
(gogoproto.nullable) = false
];

// Block timestamp for the block where the oracle came to consensus for this
// price. This timestamp is a conventional Unix millisecond time, i.e. the
// number of milliseconds elapsed since January 1, 1970 UTC.
int64 block_timestamp_ms = 2;

// Block height when the oracle came to consensus for this price.
uint64 block_height = 3;
}

// QueryExchangeRatesResponse is response type for the
// Query/ExchangeRates RPC method.
Expand Down
16 changes: 13 additions & 3 deletions x/evm/embeds/artifacts/contracts/IOracle.sol/IOracle.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,19 @@
"name": "queryExchangeRate",
"outputs": [
{
"internalType": "string",
"name": "",
"type": "string"
"internalType": "uint256",
"name": "price",
"type": "uint256"
},
{
"internalType": "uint64",
"name": "blockTimeMs",
"type": "uint64"
},
{
"internalType": "uint64",
"name": "blockHeight",
"type": "uint64"
}
],
"stateMutability": "view",
Expand Down
20 changes: 14 additions & 6 deletions x/evm/embeds/contracts/IOracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,20 @@ pragma solidity >=0.8.19;

/// @notice Oracle interface for querying exchange rates
interface IOracle {
/// @notice Queries the exchange rate for a given pair
/// @param pair The asset pair to query. For example, "ubtc:uusd" is the
/// USD price of BTC and "unibi:uusd" is the USD price of NIBI.
/// @return The exchange rate (a decimal value) as a string.
/// @dev This function is view-only and does not modify state.
function queryExchangeRate(string memory pair) external view returns (string memory);
/// @notice Queries the dated exchange rate for a given pair
/// @param pair The asset pair to query. For example, "ubtc:uusd" is the
/// USD price of BTC and "unibi:uusd" is the USD price of NIBI.
/// @return price The exchange rate for the given pair
/// @return blockTimeMs The block time in milliseconds when the price was
/// last updated
/// @return blockHeight The block height when the price was last updated
/// @dev This function is view-only and does not modify state.
function queryExchangeRate(
string memory pair
)
external
view
returns (uint256 price, uint64 blockTimeMs, uint64 blockHeight);
}

address constant ORACLE_PRECOMPILE_ADDRESS = 0x0000000000000000000000000000000000000801;
Expand Down
4 changes: 2 additions & 2 deletions x/evm/precompile/oracle.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,12 @@ func (p precompileOracle) queryExchangeRate(
return nil, err
}

price, err := p.oracleKeeper.GetExchangeRate(ctx, assetPair)
price, blockTime, blockHeight, err := p.oracleKeeper.GetDatedExchangeRate(ctx, assetPair)
if err != nil {
return nil, err
}

return method.Outputs.Pack(price.String())
return method.Outputs.Pack(price.BigInt(), uint64(blockTime), blockHeight)
}

func (p precompileOracle) parseQueryExchangeRateArgs(args []any) (
Expand Down
8 changes: 7 additions & 1 deletion x/evm/precompile/oracle_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package precompile_test

import (
"math/big"
"testing"
"time"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -55,7 +57,9 @@ func (s *OracleSuite) TestOracle_HappyPath() {

s.T().Log("Query exchange rate")
{
deps.Ctx = deps.Ctx.WithBlockTime(time.Unix(69, 420)).WithBlockHeight(69)
deps.App.OracleKeeper.SetPrice(deps.Ctx, "unibi:uusd", sdk.MustNewDecFromStr("0.067"))

input, err := embeds.SmartContract_Oracle.ABI.Pack("queryExchangeRate", "unibi:uusd")
s.NoError(err)
resp, _, err := deps.EvmKeeper.CallContractWithInput(
Expand All @@ -68,7 +72,9 @@ func (s *OracleSuite) TestOracle_HappyPath() {
s.NoError(err)

// Check the response
s.Equal("0.067000000000000000", out[0].(string))
s.Equal(out[0].(*big.Int), big.NewInt(67000000000000000))
s.Equal(out[1].(uint64), uint64(69000))
s.Equal(out[2].(uint64), uint64(69))
Comment on lines +75 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve test maintainability with explicit value calculations.

The test assertions would be clearer with explicit calculations of expected values. This would make the relationship between inputs and expected outputs more obvious.

+// Define expected values with clear calculations
+expectedPrice := big.NewInt(67000000000000000) // 0.067 in wei
+expectedBlockTimeMs := uint64(69 * 1000)       // Unix seconds to milliseconds
+expectedBlockHeight := uint64(69)
-s.Equal(out[0].(*big.Int), big.NewInt(67000000000000000))
-s.Equal(out[1].(uint64), uint64(69000))
-s.Equal(out[2].(uint64), uint64(69))
+s.Equal(out[0].(*big.Int), expectedPrice)
+s.Equal(out[1].(uint64), expectedBlockTimeMs)
+s.Equal(out[2].(uint64), expectedBlockHeight)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
s.Equal(out[0].(*big.Int), big.NewInt(67000000000000000))
s.Equal(out[1].(uint64), uint64(69000))
s.Equal(out[2].(uint64), uint64(69))
// Define expected values with clear calculations
expectedPrice := big.NewInt(67000000000000000) // 0.067 in wei
expectedBlockTimeMs := uint64(69 * 1000) // Unix seconds to milliseconds
expectedBlockHeight := uint64(69)
s.Equal(out[0].(*big.Int), expectedPrice)
s.Equal(out[1].(uint64), expectedBlockTimeMs)
s.Equal(out[2].(uint64), expectedBlockHeight)

}
}

Expand Down
10 changes: 10 additions & 0 deletions x/oracle/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,16 @@ func (k Keeper) GetExchangeRate(ctx sdk.Context, pair asset.Pair) (price sdk.Dec
return
}

func (k Keeper) GetDatedExchangeRate(ctx sdk.Context, pair asset.Pair) (price sdk.Dec, blockTimeMs int64, BlockHeight uint64, err error) {
exchangeRate, err := k.ExchangeRates.Get(ctx, pair)
if err != nil {
return
}
Comment on lines +191 to +194
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling with specific error types.

The current error handling doesn't provide specific information about why the exchange rate retrieval failed. Consider wrapping the error with additional context.

 	exchangeRate, err := k.ExchangeRates.Get(ctx, pair)
 	if err != nil {
-		return
+		return sdk.Dec{}, 0, 0, sdkerrors.Wrapf(types.ErrNoValidPrice, "failed to get exchange rate for pair %s: %v", pair, err)
 	}

Committable suggestion was skipped due to low confidence.

time := ctx.WithBlockHeight(int64(exchangeRate.CreatedBlock)).BlockTime()
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved

return exchangeRate.ExchangeRate, time.UnixMilli(), exchangeRate.CreatedBlock, nil
}

// SetPrice sets the price for a pair as well as the price snapshot.
func (k Keeper) SetPrice(ctx sdk.Context, pair asset.Pair, price sdk.Dec) {
k.ExchangeRates.Insert(ctx, pair, types.DatedPrice{ExchangeRate: price, CreatedBlock: uint64(ctx.BlockHeight())})
Expand Down
18 changes: 18 additions & 0 deletions x/oracle/keeper/querier.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,24 @@ func (q querier) ExchangeRateTwap(c context.Context, req *types.QueryExchangeRat
return &types.QueryExchangeRateResponse{ExchangeRate: twap}, nil
}

// get the latest price snapshot from the oracle for a pair
func (q querier) DatedExchangeRate(c context.Context, req *types.QueryExchangeRateRequest) (response *types.QueryDatedExchangeRateResponse, err error) {
if _, err = q.ExchangeRate(c, req); err != nil {
return
}

ctx := sdk.UnwrapSDKContext(c)
price, blockTime, blockHeight, err := q.Keeper.GetDatedExchangeRate(ctx, req.Pair)
if err != nil {
return &types.QueryDatedExchangeRateResponse{}, err
}
return &types.QueryDatedExchangeRateResponse{
Price: price,
BlockTimestampMs: blockTime,
BlockHeight: blockHeight,
}, nil
}

// ExchangeRates queries exchange rates of all pairs
func (q querier) ExchangeRates(c context.Context, _ *types.QueryExchangeRatesRequest) (*types.QueryExchangeRatesResponse, error) {
ctx := sdk.UnwrapSDKContext(c)
Expand Down
105 changes: 105 additions & 0 deletions x/oracle/keeper/querier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,111 @@ func TestQueryExchangeRateTwap(t *testing.T) {
require.Equal(t, math.LegacyMustNewDecFromStr("1700"), res.ExchangeRate)
}

func TestQueryDatedExchangeRate(t *testing.T) {
input := CreateTestFixture(t)
querier := NewQuerier(input.OracleKeeper)

// Set initial block time and height
initialTime := input.Ctx.BlockTime()
initialHeight := input.Ctx.BlockHeight()

// Pair 1: BTC/NUSD
pairBTC := asset.Registry.Pair(denoms.BTC, denoms.NUSD)
rateBTC1 := math.LegacyNewDec(1700)
rateBTC2 := math.LegacyNewDec(1800)

// Pair 2: ETH/NUSD
pairETH := asset.Registry.Pair(denoms.ETH, denoms.NUSD)
rateETH1 := math.LegacyNewDec(100)
rateETH2 := math.LegacyNewDec(110)

Comment on lines +121 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using table-driven tests for better coverage.

While the test setup is well-structured, consider converting it to a table-driven test to cover more edge cases such as:

  • Invalid pairs
  • Zero or negative prices
  • Different time intervals
  • Maximum/minimum values

// --- Set first price for BTC/NUSD ---
input.OracleKeeper.SetPrice(input.Ctx, pairBTC, rateBTC1)
testutilevents.RequireContainsTypedEvent(
t,
input.Ctx,
&types.EventPriceUpdate{
Pair: pairBTC.String(),
Price: rateBTC1,
TimestampMs: input.Ctx.BlockTime().UnixMilli(),
},
)

// Advance time and block height
input.Ctx = input.Ctx.WithBlockTime(initialTime.Add(1 * time.Second)).
WithBlockHeight(initialHeight + 1)

// --- Set first price for ETH/NUSD ---
input.OracleKeeper.SetPrice(input.Ctx, pairETH, rateETH1)
testutilevents.RequireContainsTypedEvent(
t,
input.Ctx,
&types.EventPriceUpdate{
Pair: pairETH.String(),
Price: rateETH1,
TimestampMs: input.Ctx.BlockTime().UnixMilli(),
},
)

// Advance time and block height
input.Ctx = input.Ctx.WithBlockTime(initialTime.Add(2 * time.Second)).
WithBlockHeight(initialHeight + 2)

// --- Set second price for BTC/NUSD ---
input.OracleKeeper.SetPrice(input.Ctx, pairBTC, rateBTC2)
testutilevents.RequireContainsTypedEvent(
t,
input.Ctx,
&types.EventPriceUpdate{
Pair: pairBTC.String(),
Price: rateBTC2,
TimestampMs: input.Ctx.BlockTime().UnixMilli(),
},
)

// Advance time and block height
input.Ctx = input.Ctx.WithBlockTime(initialTime.Add(3 * time.Second)).
WithBlockHeight(initialHeight + 3)

// --- Set second price for ETH/NUSD ---
input.OracleKeeper.SetPrice(input.Ctx, pairETH, rateETH2)
testutilevents.RequireContainsTypedEvent(
t,
input.Ctx,
&types.EventPriceUpdate{
Pair: pairETH.String(),
Price: rateETH2,
TimestampMs: input.Ctx.BlockTime().UnixMilli(),
},
)
Comment on lines +166 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract helper functions to reduce code duplication.

Consider creating helper functions to reduce repetition:

+	func setPriceAndVerifyEvent(
+		t *testing.T,
+		ctx sdk.Context,
+		k Keeper,
+		pair asset.Pair,
+		price math.LegacyDec,
+	) {
+		k.SetPrice(ctx, pair, price)
+		testutilevents.RequireContainsTypedEvent(
+			t,
+			ctx,
+			&types.EventPriceUpdate{
+				Pair:        pair.String(),
+				Price:       price,
+				TimestampMs: ctx.BlockTime().UnixMilli(),
+				BlockHeight: ctx.BlockHeight(),
+			},
+		)
+	}

Committable suggestion was skipped due to low confidence.


// Wrap context for querying
ctx := sdk.WrapSDKContext(input.Ctx)

// --- Query latest snapshot for BTC/NUSD ---
resBTC, err := querier.DatedExchangeRate(
ctx,
&types.QueryExchangeRateRequest{Pair: pairBTC},
)
require.NoError(t, err)
require.Equal(t, rateBTC2, resBTC.Price)
require.Equal(t, input.Ctx.BlockTime().UnixMilli(), resBTC.BlockTimestampMs)

// --- Query latest snapshot for ETH/NUSD ---
resETH, err := querier.DatedExchangeRate(
ctx,
&types.QueryExchangeRateRequest{Pair: pairETH},
)
require.NoError(t, err)
require.Equal(t, rateETH2, resETH.Price)
require.Equal(t, input.Ctx.BlockTime().UnixMilli(), resETH.BlockTimestampMs)

// --- Query a pair with no snapshots (should return an error) ---
pairATOM := asset.Registry.Pair(denoms.ATOM, denoms.NUSD)
_, err = querier.DatedExchangeRate(ctx, &types.QueryExchangeRateRequest{Pair: pairATOM})
require.Error(t, err)
}

func TestCalcTwap(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading
Loading