genesis balance allocations and transfer transaction #392

jchappelow · 2023-11-20T23:52:01Z

./kwil-admin setup init --alloc dc18f4993e93b50486e3e54e27d91d57cee1da07=12345678900000000000000000 --alloc 4ac6d06721e51ca5ded3470893aec1d9f48fc834=987654321 --gas

./kwil-cli account transfer 4ac6d06721e51ca5ded3470893aec1d9f48fc834 12341324

./kwil-cli account balance

./kwil-cli account balance 4ac6d06721e51ca5ded3470893aec1d9f48fc834

jchappelow · 2023-11-22T15:56:02Z

core/types/transactions/payloads.go

+type Transfer struct {
+	To     []byte `json:"to"`     // to be string as user identifier
+	Amount string `json:"amount"` // big.Int
+}


@KwilLuke There's more to do with this PR before it's ready for review, but it is working and this is the payload structure, of type PayloadTypeTransfer = "transfer".

The genesis config specifies accounts (pubkey presently, but will probably change when identifier work is merged) with balances, so a faucet would just have the corresponding private key to control the funds.

kwil-cli has a transfer command and the client package has a Transfer method: https://github.com/kwilteam/kwil-db/pull/392/files#diff-c3fd86c44b5f4a4cf8bad895c77a7514c9a6b72d3c1c6da608545168bdecaf3a (just as an example, it's really simple)

@brennanjl Just to talk through the implications of the planned identifier changes on the alloc specification in genesis, we have this presently:

"genesis_time": "2023-11-20T23:39:42.725453464Z", "chain_id": "kwil-chain-KtqCimeh", "initial_height": 1, "app_hash": null, "alloc": { "040dfec0a4e644dc129ace6e40d813cd65a21874586b9c782cd1779e44f2719f11f74d7d6d76c09f281ce5588dabdd0dab3e59b7f974de4c045da8b727c4521d0b": 12345678900000000000 }, "consensus_params": { ...

That "alloc" specifies accounts with their balances at genesis. This PR is based on main, so that's public key hex. This would become something else. I suppose if that account is controlled via eth "personal_sign" then it would be the 20 bytes of the address (in json key as hex string), as would be the case since our SDKs default to that except for the validator transactions. If it is controlled with an ed25519 signer, then I think it would be the hex of the public key.

Not really seeing any problems, but wanted to describe it in the context of identifiers.

KwilLuke · 2023-11-28T21:05:41Z

In our system, how much will database deploys and action transactions cost? Will it be 1 unit, or it will it be 10^18 units like in Ethereum?

jchappelow · 2023-11-28T22:03:59Z

In our system, how much will database deploys and action transactions cost? Will it be 1 unit, or it will it be 10^18 units like in Ethereum?

Presently it's all hard coded, but we're still using "wei" with lots of zeros:

var (
	defaultDeployPrice  = big.NewInt(1000000000000000000)
	defaultDropPrice    = big.NewInt(10000000000000)
	defaultExecutePrice = big.NewInt(2000000000000000)
)

KwilLuke · 2023-11-29T21:28:26Z

Okay - if each user requests 10 * 10^18 tokens, we will probably just want to make sure we initialize the test network with more than a sufficient number of tokens. Maybe 1 billion * 10^18?

jchappelow · 2023-11-30T15:19:01Z

Needs some cleanup, but I've checked all the boxes and it's working (for Luke and acceptance tests), so making ready for review.

jchappelow · 2023-11-30T16:03:20Z

test/acceptance/kwild_test.go


 var drivers = flag.String("drivers", "http,cli", "comma separated list of drivers to run")

+func TestKwildTransferAcceptance(t *testing.T) {


@Yaiba The approach here is very close with what you described in the README. Only difference is that I didn't see a purpose to defining a specification struct that would provide these methods, rather just convert the driver into the specific spec being tested.

Also I made an exported ExpectTxSuccess helper, which was really the only thing in a hypothetical spec struct that would not just be a passthrough to the driver.

This will do, at least for acceptance test. The idea of a spec is mostly make a behavior into a thingy for reuse.
In this test transfer case, you transfer, you expect balance changes on sender and receiver.
Any other tests requires transfer would just simple reuse this spec. For example, if you also need to transfer in integration test, a spec will be handy

Looking at https://github.com/kwilteam/kwil-db/blob/main/test/specifications/README.md, that suggest it's the Dsls that are reusable, not the specs. I'm confused now.
It becomes difficult to write a spec that does not have a bunch of parameters as input args, so it looked from the README that the suggestion was to have the test case itself use the DSLs and implement the spec right there. Note how the concrete spec you've suggested is just wrappers.

internal/modules/errors.go

jchappelow · 2023-11-30T16:08:01Z

internal/modules/accounts/accounts.go

+// TransferTx is used to handle a transfer transaction. This involves:
+//   - check that transaction fee is adequate
+//   - pay for the transaction gas, updating balance and nonce in the DB
+//   - transfer value from the sender account to the recipient account
+//
+// The blockchain application will have decoded the transaction and payload, and
+// passed only the required data via TxAcct.
+func (am *AccountsModule) TransferTx(ctx context.Context, tx *TxAcct, to []byte, amt *big.Int) error {


This and PriceTransfer are the main reasons why it felt like there needed to be an actual blockchain module rather than having the account store being directly used by the ABCI app. I'm trying to keep the account store focused on DB-related concerns. The existing Spend method there gets a little too concerned with nonces and such in this view, but I didn't want to change more than necessary.

jchappelow · 2023-11-30T16:08:52Z

internal/modules/datasets/execution.go

@@ -116,6 +119,6 @@ func (u *DatasetModule) compareAndSpend(ctx context.Context, price *big.Int, tx
 func resp(fee *big.Int) *ExecutionResponse {
 	return &ExecutionResponse{
 		Fee:     fee,
-		GasUsed: 0,
+		GasUsed: 0, // !?


I need to look back at this. This field looks to be useless and I think there was an intention of using it to convey non-zero values back to abci.

Yeah it was added but not used for anything b/c we do not have gas metering. Can remove in this pr

internal/accounts/opts.go

jchappelow · 2023-11-30T21:08:22Z

test/acceptance/kwild_test.go

+
+		t.Run(driverType+"_driver", func(t *testing.T) {
+			senderDriver := helper.GetDriver(driverType, "creator")
+			sender := specifications.TransferAmountDsl(senderDriver)


Here's the "spec" that would be reusable. There's no need to make a concrete type that wraps the driver, just convert to the interface.

I think this makes a lot of sense, however is it worth while to try to maintain the previous way in which Gavin did it?

The only reason I bring this up is because it makes it easier to replicate in integration tests. I would imagine that we would want to check that some functionality (like Transfer) would not cause non-determinism / a fork. Reusing the logic in this test could be helpful there.

No strong opinion though, mostly just trying to consider all angles.

I'm a little lost on this spec business TBH. @Yaiba had noted that we haven't been doing it right currently with the one big test case, and he documented the intended way we should gravitate towards here with a test case using one spec: https://github.com/kwilteam/kwil-db/blob/main/test/specifications/README.md

I was actually trying to follow that new example. What was baffling to me is that the "spec" in that README is nothing at all, just a wrapper for the Dsl, which is concretely the "driver". So we have the reusable drivers in test/driver and the specifications.TransferAmountDsl interface in test/specifications. then in test/intergration we can do like the following in say TestKwildValidatorUpdatesIntegration:

transfer := specifications.TransferAmountDsl(node0Driver) transfer.TransferAmt(ctx, joinerPrivKey.Bytes(), big.NewInt(1234567890))

I think I need to see this refactored by @Yaiba to understand the spec/dsl/driver design that's intended. 😆

Sorry I didn't follow this topic soon enough. Yeah I was going to update the spec readme and it's stashed... I'll update it sooner.

jchappelow · 2023-12-04T22:31:12Z

I'll squash down more before we merge this. Most of the commits are distinct for easier review though, only a couple stray fixups. Ideally I would have kept squashing as I went went with this, but I gotta switch to review now.

internal/abci/abci.go

brennanjl · 2023-12-05T16:19:55Z

internal/modules/accounts/accounts.go

+	// Pay for gas (the tx fee), and increment nonce.
+	spend := &accounts.Spend{
+		AccountID: tx.Sender,
+		Amount:    transferPrice, // not the Fee???


Just responding to the in-line comment here:

We want it to be the transferPrice and not fee because the user might by overly generous with the free.

For example, if you agree to pay 500 gas, but it only costs 400, the user should only be debited 400. Obviously that probably would not happen with a gas system as simple as ours right now, but in the case of dynamic gas it would.

Related issue discussing this point: #410

brennanjl · 2023-12-05T16:22:04Z

internal/modules/datasets/execution.go

@@ -116,6 +119,6 @@ func (u *DatasetModule) compareAndSpend(ctx context.Context, price *big.Int, tx
 func resp(fee *big.Int) *ExecutionResponse {
 	return &ExecutionResponse{
 		Fee:     fee,
-		GasUsed: 0,
+		GasUsed: 0, // !?


Yeah it was added but not used for anything b/c we do not have gas metering. Can remove in this pr

brennanjl · 2023-12-05T16:28:29Z

test/acceptance/kwild_test.go

+
+		t.Run(driverType+"_driver", func(t *testing.T) {
+			senderDriver := helper.GetDriver(driverType, "creator")
+			sender := specifications.TransferAmountDsl(senderDriver)


I think this makes a lot of sense, however is it worth while to try to maintain the previous way in which Gavin did it?

The only reason I bring this up is because it makes it easier to replicate in integration tests. I would imagine that we would want to check that some functionality (like Transfer) would not cause non-determinism / a fork. Reusing the logic in this test could be helpful there.

No strong opinion though, mostly just trying to consider all angles.

- Add genesis allocs a la ethereum genesis.json. - Add a transfer tx payload. - client,kwil-cli: add accounts command inc. transfer and balance - Create an accounts blockchain module. - rpc,client: fix errors, http's parseBroadcastResponse, and waittx - test: add acceptance test and require gas in validator integ test

* workable gas costs with genesis allocs and transfer txn - Add genesis allocs a la ethereum genesis.json. - Add a transfer tx payload. - client,kwil-cli: add accounts command inc. transfer and balance - Create an accounts blockchain module. - rpc,client: fix errors, http's parseBroadcastResponse, and waittx - test: add acceptance test and require gas in validator integ test * special case for eth 0x addresses in genesis allocs * admin: default output-dir for setup cmds to .testnet * fix setup testnet defaults * use GenericSwaggerError to get rpc error details

jchappelow changed the title ~~[WIP] gas treasury~~ genesis balance allocations and transfer transaction Nov 21, 2023

jchappelow commented Nov 22, 2023

View reviewed changes

jchappelow force-pushed the gas-treasury branch 4 times, most recently from 7eb34d8 to aafa64b Compare November 27, 2023 23:36

jchappelow force-pushed the gas-treasury branch from 2af1b88 to 1b1dd87 Compare November 28, 2023 21:56

jchappelow force-pushed the gas-treasury branch 3 times, most recently from 0a8bb44 to 6542d15 Compare November 29, 2023 23:28

jchappelow marked this pull request as ready for review November 30, 2023 15:19

jchappelow commented Nov 30, 2023

View reviewed changes

jchappelow force-pushed the gas-treasury branch 2 times, most recently from 9171d30 to eec535c Compare November 30, 2023 17:52

jchappelow commented Nov 30, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

jchappelow force-pushed the gas-treasury branch from eec535c to ee10b53 Compare December 1, 2023 21:04

jchappelow added this to the v0.6.0 milestone Dec 4, 2023

jchappelow mentioned this pull request Dec 4, 2023

query cost, gas, and fees #410

Open

9 tasks

jchappelow force-pushed the gas-treasury branch from dfdd4d8 to d850e20 Compare December 5, 2023 02:17

charithabandi approved these changes Dec 5, 2023

View reviewed changes

brennanjl reviewed Dec 5, 2023

View reviewed changes

jchappelow force-pushed the gas-treasury branch 2 times, most recently from d9bd9d4 to 001ae13 Compare December 5, 2023 23:18

jchappelow force-pushed the gas-treasury branch from 0b2b04e to 4dfc17c Compare December 6, 2023 00:06

jchappelow force-pushed the gas-treasury branch from 4dfc17c to d11d3de Compare December 6, 2023 00:18

jchappelow added 4 commits December 5, 2023 18:20

special case for eth 0x addresses in genesis allocs

ac0c566

admin: default output-dir for setup cmds to .testnet

d0f1d26

fix setup testnet defaults

e1e16c5

use GenericSwaggerError to get rpc error details

b060587

brennanjl merged commit e014b0a into main Dec 6, 2023
2 checks passed

brennanjl deleted the gas-treasury branch December 6, 2023 05:14

Yaiba mentioned this pull request Jan 8, 2024

Test/kgw #446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

genesis balance allocations and transfer transaction #392

genesis balance allocations and transfer transaction #392

jchappelow commented Nov 20, 2023 •

edited

Loading

jchappelow Nov 22, 2023

jchappelow Nov 22, 2023

KwilLuke commented Nov 28, 2023

jchappelow commented Nov 28, 2023

KwilLuke commented Nov 29, 2023 •

edited

Loading

jchappelow commented Nov 30, 2023

jchappelow Nov 30, 2023 •

edited

Loading

Yaiba Nov 30, 2023

jchappelow Nov 30, 2023

jchappelow Nov 30, 2023

jchappelow Nov 30, 2023

brennanjl Dec 5, 2023

jchappelow Nov 30, 2023 •

edited

Loading

brennanjl Dec 5, 2023

jchappelow Dec 5, 2023

Yaiba Dec 6, 2023

This comment was marked as resolved.

jchappelow commented Dec 4, 2023

brennanjl Dec 5, 2023

jchappelow Dec 5, 2023

brennanjl Dec 5, 2023

brennanjl Dec 5, 2023


		var drivers = flag.String("drivers", "http,cli", "comma separated list of drivers to run")

		func TestKwildTransferAcceptance(t *testing.T) {

genesis balance allocations and transfer transaction #392

genesis balance allocations and transfer transaction #392

Conversation

jchappelow commented Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KwilLuke commented Nov 28, 2023

jchappelow commented Nov 28, 2023

KwilLuke commented Nov 29, 2023 • edited Loading

jchappelow commented Nov 30, 2023

jchappelow Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

jchappelow commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow commented Nov 20, 2023 •

edited

Loading

KwilLuke commented Nov 29, 2023 •

edited

Loading

jchappelow Nov 30, 2023 •

edited

Loading

jchappelow Nov 30, 2023 •

edited

Loading