Skip to content

Commit

Permalink
fix: Fix starknet_getStorageAt handler in edge cases (#2464)
Browse files Browse the repository at this point in the history 
* fix: Fix starknet_getStorageAt handler in edge cases

* nit: Give clearer var name
  • Loading branch information
@hudem1
hudem1 authored Feb 13, 2025
1 parent 6248280 commit e831309
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 59 deletions.
20 changes: 18 additions & 2 deletions rpc/v6/contract.go
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package rpcv6

import (
"errors"

"github.com/NethermindEth/juno/core/felt"
"github.com/NethermindEth/juno/db"
"github.com/NethermindEth/juno/jsonrpc"
rpccore "github.com/NethermindEth/juno/rpc/rpccore"
)
Expand Down Expand Up @@ -29,7 +32,7 @@ func (h *Handler) Nonce(id BlockID, address felt.Felt) (*felt.Felt, *jsonrpc.Err
return nonce, nil
}

// StorageAt gets the value of the storage at the given address and key.
// StorageAt gets the value of the storage at the given address and key for a given block.
//
// It follows the specification defined here:
// https://github.com/starkware-libs/starknet-specs/blob/a789ccc3432c57777beceaa53a34a7ae2f25fda0/api/starknet_api_openrpc.json#L110
Expand All @@ -40,9 +43,22 @@ func (h *Handler) StorageAt(address, key felt.Felt, id BlockID) (*felt.Felt, *js
}
defer h.callAndLogErr(stateCloser, "Error closing state reader in getStorageAt")

// Check if the contract exists because if it doesn't, the call to `ContractStorage` below
// will just return a zero-valued felt with a nil error (not an `ErrContractNotFound` error)
_, err := stateReader.ContractClassHash(&address)
if err != nil {
if errors.Is(err, db.ErrKeyNotFound) {
return nil, rpccore.ErrContractNotFound
}
h.log.Errorw("Failed to get contract class hash", "err", err)
return nil, rpccore.ErrInternal
}

// Check if key exists in existing contract
// If it doesn't, it will return a zero-valued felt with a nil error
value, err := stateReader.ContractStorage(&address, &key)
if err != nil {
return nil, rpccore.ErrContractNotFound
return nil, rpccore.ErrInternal
}

return value, nil
Expand Down
61 changes: 42 additions & 19 deletions rpc/v6/contract_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,73 +101,96 @@ func TestStorageAt(t *testing.T) {
t.Run("empty blockchain", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

t.Run("non-existent block hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

t.Run("non-existent block number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

mockState := mocks.NewMockStateHistoryReader(mockCtrl)

t.Run("non-existent contract", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(nil, errors.New("non-existent contract"))
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrContractNotFound, rpcErr)
})

t.Run("internal error while retrieving contract", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, errors.New("some internal error"))

storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrInternal, rpcErr)
})

t.Run("non-existent key", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(&felt.Zero, errors.New("non-existent key"))
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(&felt.Zero, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storage)
assert.Equal(t, rpccore.ErrContractNotFound, rpcErr)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Equal(t, storageValue, &felt.Zero)
assert.Nil(t, rpcErr)
})

t.Run("internal error while retrieving key", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(nil, errors.New("some internal error"))

storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrInternal, rpcErr)
})

expectedStorage := new(felt.Felt).SetUint64(1)

t.Run("blockID - latest", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})

t.Run("blockID - hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})

t.Run("blockID - number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})
}
2 changes: 1 addition & 1 deletion rpc/v7/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (h *Handler) StorageAt(address, key felt.Felt, id BlockID) (*felt.Felt, *js

value, err := stateReader.ContractStorage(&address, &key)
if err != nil {
return nil, rpccore.ErrContractNotFound
return nil, rpccore.ErrInternal
}

return value, nil
Expand Down
46 changes: 28 additions & 18 deletions rpc/v7/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,24 @@ func TestStorageAt(t *testing.T) {
t.Run("empty blockchain", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

t.Run("non-existent block hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Hash: &felt.Zero})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Hash: &felt.Zero})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

t.Run("non-existent block number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Number: 0})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Number: 0})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

Expand All @@ -52,19 +52,29 @@ func TestStorageAt(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrContractNotFound, rpcErr)
})

t.Run("non-existent key", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(nil, db.ErrKeyNotFound)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(&felt.Zero, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, storage)
assert.Equal(t, rpccore.ErrContractNotFound, rpcErr)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Equal(t, storageValue, &felt.Zero)
assert.Nil(t, rpcErr)
})

t.Run("internal error while retrieving key", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(nil, errors.New("some internal error"))

Check failure on line 73 in rpc/v7/storage_test.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: errors (typecheck)

Check failure on line 73 in rpc/v7/storage_test.go

View workflow job for this annotation

GitHub Actions / Run Tests (ubuntu-latest) 

undefined: errors

Check failure on line 73 in rpc/v7/storage_test.go

View workflow job for this annotation

GitHub Actions / Run Tests (macos-latest) 

undefined: errors

Check failure on line 73 in rpc/v7/storage_test.go

View workflow job for this annotation

GitHub Actions / Run Tests (ubuntu-arm64-4-core) 

undefined: errors

storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrInternal, rpcErr)
})

expectedStorage := new(felt.Felt).SetUint64(1)
Expand All @@ -74,28 +84,28 @@ func TestStorageAt(t *testing.T) {
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Latest: true})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})

t.Run("blockID - hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Hash: &felt.Zero})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Hash: &felt.Zero})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})

t.Run("blockID - number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Number: 0})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpcv7.BlockID{Number: 0})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})
}
2 changes: 1 addition & 1 deletion rpc/v8/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (h *Handler) StorageAt(address, key felt.Felt, id BlockID) (*felt.Felt, *js

value, err := stateReader.ContractStorage(&address, &key)
if err != nil {
return nil, rpccore.ErrContractNotFound
return nil, rpccore.ErrInternal
}

return value, nil
Expand Down
46 changes: 28 additions & 18 deletions rpc/v8/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,24 +36,24 @@ func TestStorageAt(t *testing.T) {
t.Run("empty blockchain", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

t.Run("non-existent block hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

t.Run("non-existent block number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(nil, nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrBlockNotFound, rpcErr)
})

Expand All @@ -63,19 +63,29 @@ func TestStorageAt(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(gomock.Any()).Return(nil, db.ErrKeyNotFound)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storage)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrContractNotFound, rpcErr)
})

t.Run("non-existent key", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(nil, db.ErrKeyNotFound)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(&felt.Zero, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, storage)
assert.Equal(t, rpccore.ErrContractNotFound, rpcErr)
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
assert.Equal(t, &felt.Zero, storageValue)
require.Nil(t, rpcErr)
})

t.Run("internal error while retrieving key", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(nil, errors.New("some internal error"))

Check failure on line 84 in rpc/v8/storage_test.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: errors (typecheck)

Check failure on line 84 in rpc/v8/storage_test.go

View workflow job for this annotation

GitHub Actions / Run Tests (ubuntu-latest) 

undefined: errors

Check failure on line 84 in rpc/v8/storage_test.go

View workflow job for this annotation

GitHub Actions / Run Tests (macos-latest) 

undefined: errors

Check failure on line 84 in rpc/v8/storage_test.go

View workflow job for this annotation

GitHub Actions / Run Tests (ubuntu-arm64-4-core) 

undefined: errors

storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
assert.Nil(t, storageValue)
assert.Equal(t, rpccore.ErrInternal, rpcErr)
})

expectedStorage := new(felt.Felt).SetUint64(1)
Expand All @@ -85,29 +95,29 @@ func TestStorageAt(t *testing.T) {
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Latest: true})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})

t.Run("blockID - hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Hash: &felt.Zero})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})

t.Run("blockID - number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(nil, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

storage, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
storageValue, rpcErr := handler.StorageAt(felt.Zero, felt.Zero, rpc.BlockID{Number: 0})
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, storage)
assert.Equal(t, expectedStorage, storageValue)
})
}

Expand Down

0 comments on commit e831309

Please sign in to comment.