Beds 526/Raw DB sdk #947

Tangui-Bitfly · 2024-10-14T10:02:59Z

add db2.BigTableEthRaw which only purpose is reading from the raw bigtable store
db2.BigTableEthRaw is a http.RoundTriper, this allow the caller to use when creating a new ethclient.Client
introduce the db2.RawStore to handle all interaction with bigtable satore. There are some duplications here, the aim of this pr is not to refactor everything, it's just to provide an easy to use way of retrieving eth data from bigtable
it is used as follow

bt, err := store.NewBigTableWithClient(context.Background(), client, admin, raw)
if err != nil {
	t.Fatal(err)
}

rawStore := NewRawStore(store.Wrap(bt, BlocRawTable, ""))

rpcClient, err := rpc.DialOptions(context.Background(), "http://foo.bar", rpc.WithHTTPClient(&http.Client{
	Transport: NewBigTableEthRaw(rawStore, chainID),
}))
ethClient := ethclient.NewClient(rpcClient)

backend/internal/contracts/README.md

backend/internal/th/ethereum.go

backend/pkg/commons/db2/database/bigtable.go

backend/pkg/commons/db2/database/bigtable_test.go

Monika-Bitfly · 2024-11-19T17:49:50Z

backend/pkg/commons/db2/databasetest/bigtable.go

+	if err != nil {
+		t.Fatal(err)
+	}
+	ctx := context.Background()


Suggested change

ctx := context.Background()

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)

defer cancel()

It's databasetest/bigtable.go is the sim bigtable but controlled through go. I don't really see the need for the context with timeout in that case.

Monika-Bitfly · 2024-11-19T18:04:54Z

backend/pkg/commons/db2/parser.go

+}
+
+func (s *SenderFromDBSigner) ChainID() *big.Int {
+	panic("can't sign with SenderFromDBSigner")


Suggested change

panic("can't sign with SenderFromDBSigner")

panic("can't sign with SenderFromDBSigner: ChainID method not supported")

I prefer to modify as little as possible when we "copy" code from somewhere.
The reasoning behind that is: let say in the future the source is update, we propagate this update, but we loose the local change.
So we may introduce a behavior that is prone to change.

Monika-Bitfly · 2024-11-19T18:05:22Z

backend/pkg/commons/db2/parser.go

+	panic("can't sign with SenderFromDBSigner")
+}
+func (s *SenderFromDBSigner) Hash(tx *types.Transaction) common.Hash {
+	panic("can't sign with SenderFromDBSigner")


Suggested change

panic("can't sign with SenderFromDBSigner")

panic("can't sign with SenderFromDBSigner: Hash method not supported")

Monika-Bitfly · 2024-11-19T18:08:01Z

backend/pkg/commons/db2/parser.go

+	panic("can't sign with SenderFromDBSigner")
+}
+func (s *SenderFromDBSigner) SignatureValues(tx *types.Transaction, sig []byte) (R, S, V *big.Int, err error) {
+	panic("can't sign with SenderFromDBSigner")


Suggested change

panic("can't sign with SenderFromDBSigner")

panic("can't sign with SenderFromDBSigner: SignatureValues method not supported")

Monika-Bitfly · 2024-11-19T18:11:10Z

backend/pkg/commons/db2/parser.go

+// copy of senderFromServer
+// https://github.com/ethereum/go-ethereum/blob/v1.14.11/ethclient/signer.go#L30
+type SenderFromDBSigner struct {
+	addr      common.Address


Suggested change

addr common.Address

Address common.Address

Monika-Bitfly · 2024-11-19T18:11:19Z

backend/pkg/commons/db2/parser.go

+// https://github.com/ethereum/go-ethereum/blob/v1.14.11/ethclient/signer.go#L30
+type SenderFromDBSigner struct {
+	addr      common.Address
+	Blockhash common.Hash


Suggested change

Blockhash common.Hash

BlockHash common.Hash

Monika-Bitfly · 2024-11-19T18:12:56Z

backend/pkg/commons/db2/parser.go

+	Input               string
+	Output              string
+	Error               string
+	RevertReason        string // todo have a look at this, it could improve revert message


Suggested change

RevertReason string // todo have a look at this, it could improve revert message

RevertReason string // todo have a look at this, it could improve revert message

Is this comment still needed or can we remove it?

Yes it for future use. That field provide more context but is not present everywhere. If we change the indexer I want us to think about parsing this field.

Monika-Bitfly · 2024-11-19T18:23:50Z

@Tangui-Bitfly Can you update the PR title to align with the template and to reflect the issue being addressed? Thanks! ^^

Monika-Bitfly · 2024-11-20T09:32:05Z

backend/pkg/commons/db2/raw/cache.go

+	}
+	// TODO make warning not found in cache
+	block, err := c.store.ReadBlockByNumber(chainID, number)
+	if block != nil {


Suggested change

if block != nil {

if err != nil {

return nil, fmt.Errorf("block not found in cache: error %v", err)

}

if block != nil {

No you missed the point here. We don't want to handle the err, it's a cache layer, a passthrough. But we want to save the block in cache if it's not nil.

Monika-Bitfly · 2024-11-20T10:15:38Z

backend/pkg/commons/db2/raw/cache.go

+		go c.unCacheBlockAfter(key, "", oneBlockTTL)
+		return v.(*FullBlockData), nil
+	}
+	// TODO make warning not found in cache


Suggested change

// TODO make warning not found in cache

No once we have a proper logger I want to log something here

backend/pkg/commons/db2/raw/client.go

Monika-Bitfly · 2024-11-20T11:06:30Z

backend/pkg/commons/db2/raw/client_test.go

+		block int64
+	}{
+		{
+			name:  "test block",


The test names could be more descriptive - eg this one we can consider renaming to "Test Block Retrieval" or something similar for better clarity

I prefer to keep it simple.
TestBigTableClient.test_block provide the same amount of info than TestBigTableClient.Test_Block_Retrieval
Or we could rename the test itself rather than the case.
But TestBigTableClient is describing what the test does, so I don't really how to improve it

backend/pkg/commons/db2/raw/client_test.go

db2/BigTableEthRaw: add chainID db2/BigTableEthRaw: handle index for get uncle db2/BigTableEthRaw: fix with empty tx db2/BigTableEthRaw: add real condition test db2/BigTableEthRaw: add WithFallback db2/BigTableEthRaw: fallback for unsupported method db2: add cache to raw store (#966) db2: better cache + client + update store bigtable + clean up db2/raw: add EthParse

Tangui-Bitfly · 2024-11-20T15:44:47Z

@Tangui-Bitfly Can you update the PR title to align with the template and to reflect the issue being addressed? Thanks! ^^

I'm not sure by what you want me to replace it with. There is no rule on PR name.

Monika-Bitfly · 2024-11-20T17:57:22Z

@Tangui-Bitfly Could you please update the PR title to better reflect what this PR is implementing? If you're unsure, you can refer to the doc created by @peterbitfly or other open PRs in this repo eg. PR #1144 for guidance on how to name it. Thanks!

Tangui-Bitfly · 2024-11-21T07:43:20Z

@Tangui-Bitfly Could you please update the PR title to better reflect what this PR is implementing? If you're unsure, you can refer to the doc created by @peterbitfly or other open PRs in this repo eg. PR #1144 for guidance on how to name it. Thanks!

There is no guidance about PR name. Moreover PR are mostly irrelevant now because we don't have merge commit.

Monika-Bitfly · 2024-11-21T11:02:50Z

@Tangui-Bitfly Could you please update the PR title to better reflect what this PR is implementing? If you're unsure, you can refer to the doc created by @peterbitfly or other open PRs in this repo eg. PR #1144 for guidance on how to name it. Thanks!

There is no guidance about PR name. Moreover PR are mostly irrelevant now because we don't have merge commit.

I’m not quite sure I understand how PRs are irrelevant now. A good PR title should clearly reflect the changes whether it’s a new feature or a fix. Right now the title doesn’t quite capture what this PR implements or addresses so it would be helpful if you could update it. This will make things clearer for everyone. Here are a couple of articles that can help guide you:
https://graphite.dev/blog/the-best-pr-title-of-all-time
https://blog.montrealanalytics.com/4-tips-for-effective-pull-request-naming-f60793998f04

Let me know if you have any questions, thanks!

Tangui-Bitfly force-pushed the BEDS-526/fix-internal-tx branch 3 times, most recently from e0b6374 to 1fdf888 Compare October 17, 2024 07:18

Tangui-Bitfly force-pushed the BEDS-526/fix-internal-tx branch 7 times, most recently from 86d3841 to cd8bbcb Compare November 6, 2024 14:01

Tangui-Bitfly force-pushed the BEDS-526/fix-internal-tx branch 4 times, most recently from c0ec2bc to 2657fae Compare November 18, 2024 08:10

Tangui-Bitfly requested a review from Monika-Bitfly November 18, 2024 12:07

Tangui-Bitfly changed the title ~~Beds 526/fix internal tx~~ Beds 526/Raw DB sdk Nov 19, 2024