Improve gas interface #3428

grarco · 2024-06-19T17:22:15Z

Describe your changes

Closes #3427.
Closes #3395.

Introduces a new WholeGas type to better distinguish between gas in subunits (for tracking) and gas in whole units (for events and fees).

Reduces the gas scale in the localnet genesis file and in tests by an order of magnitude to increase the gas cost of transactions which was too low.

Removes the redundant gas_used field of TxResult (since we already emit an attribute GasUsed) and collapse the previous BatchResults directly into TxResult.

Indicate on which release or other PRs this topic is based on

v0.40.0

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

codecov · 2024-06-19T20:46:41Z

Codecov Report

Attention: Patch coverage is 73.17073% with 44 lines in your changes missing coverage. Please review.

Project coverage is 53.46%. Comparing base (8479d38) to head (826c2fc).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/node/src/bench_utils.rs	0.00%	13 Missing ⚠️
crates/namada/src/ledger/protocol/mod.rs	70.00%	9 Missing ⚠️
crates/gas/src/lib.rs	73.68%	5 Missing ⚠️
crates/sdk/src/rpc.rs	0.00%	5 Missing ⚠️
crates/tx/src/data/mod.rs	91.07%	5 Missing ⚠️
crates/tx/src/data/wrapper.rs	62.50%	3 Missing ⚠️
crates/node/src/shell/finalize_block.rs	87.50%	2 Missing ⚠️
crates/light_sdk/src/reading/asynchronous/tx.rs	0.00%	1 Missing ⚠️
crates/sdk/src/tx.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3428      +/-   ##
==========================================
- Coverage   53.48%   53.46%   -0.02%     
==========================================
  Files         320      320              
  Lines      110000   109974      -26     
==========================================
- Hits        58832    58803      -29     
- Misses      51168    51171       +3

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

grarco · 2024-06-20T11:36:55Z

@yito88 Hermes tests failing again (sorry about that)

grarco · 2024-06-20T11:46:34Z

@yito88 Hermes tests failing again (sorry about that)

Ah actually I believe the failures come from #3391

yito88 · 2024-06-20T12:58:31Z

yeah, it seems that the parameter structure change in #3391 causes the failure.

grarco · 2024-06-20T14:21:56Z

yeah, it seems that the parameter structure change in #3391 causes the failure.

Maybe not, I've just pushed a fix to that branch that was supposed to fix a failing ibc test (non-hermes) but it seems to have fixed all the e2e tests. So the issue might indeed come from this branch

brentstone · 2024-07-05T22:45:01Z

do we still want this PR?

grarco · 2024-07-08T16:27:37Z

do we still want this PR?

I think it would be nice if we could add it to the next release

yito88 · 2024-07-09T09:12:04Z

@grarco Looks good. e2e::ibc_tests::run_ledger_ibc failed without Hermes. It seems that the default gas limit isn't enough for tx_ibc.wasm.

grarco · 2024-07-09T10:22:01Z

@grarco Looks good. e2e::ibc_tests::run_ledger_ibc failed without Hermes. It seems that the default gas limit isn't enough for tx_ibc.wasm.

Thanks for catching it! Should be fixed in fb81246. I've also fixed the broken integration tests

… integration tests

crates/sdk/src/tx.rs

brentstone · 2024-07-09T21:26:29Z

crates/tx/src/data/mod.rs

-            write!(f, "Transaction is valid. Gas used: {}", self.gas_used)
+            write!(f, "Transaction is valid.")


My opinion is that keeping this log is important if we are able to still.

I've had a chat with @Fraccaman on this. The gas is already emitted as an event, so this field in the TxResult was essentially duplicated. I'll try to update the logs in the client to retrieve this piece of information from the events

Brought back the logging of the gas used in 209da0c but directly in the SDK when fetching the tx events. It is no longer possible to log it directly in here since the gas isn't present here anymore

* grarco/better-gas-interface: Logs tx used gas Increases masp gas limit in genesis files and removes custom setup in integration tests update Hermes Returns `WholeGas` from `dry_run_tx` Adds conversion from `WholeGas` to `GasLimit` Updates gas in ibc e2e test Fixes gas payment in masp integration tests Updates gas limit in unit tests Changelog #3428 Changelog #3428 Removes misleading gas message Fixes ibc e2e test Fixes unit tests Clippy + fmt Compacts `BatchResults` into `TxResult` Remove gas from `TxResult`. Adjusts dry run result Reduces gas scale param in genesis Introduces `WholeGas` type

grarco added a commit that referenced this pull request Jun 20, 2024

Changelog #3428

2e55958

grarco force-pushed the grarco/better-gas-interface branch from 8f4678c to 2e55958 Compare June 20, 2024 10:40

grarco added a commit that referenced this pull request Jun 20, 2024

Changelog #3428

3d0b9cb

grarco force-pushed the grarco/better-gas-interface branch from 2e55958 to 3d0b9cb Compare June 20, 2024 10:46

grarco marked this pull request as ready for review June 20, 2024 11:36

grarco requested review from tzemanovic, brentstone, Fraccaman and yito88 June 20, 2024 11:36

grarco added a commit that referenced this pull request Jun 20, 2024

Changelog #3428

be85e27

grarco force-pushed the grarco/better-gas-interface branch from 3d0b9cb to be85e27 Compare June 20, 2024 13:46

grarco force-pushed the grarco/better-gas-interface branch from 7ea3a8b to 46dd71e Compare June 20, 2024 18:33

grarco added 9 commits June 24, 2024 14:59

Introduces WholeGas type

42556d4

Reduces gas scale param in genesis

d1dfd68

Remove gas from TxResult. Adjusts dry run result

e1bd249

Compacts BatchResults into TxResult

c8c70f2

Clippy + fmt

719ed76

Fixes unit tests

fe080b5

Fixes ibc e2e test

ecfbdfe

Removes misleading gas message

6725c0d

Changelog #3428

1dd9399

grarco force-pushed the grarco/better-gas-interface branch from 46dd71e to 1dd9399 Compare June 24, 2024 12:59

brentstone mentioned this pull request Jul 7, 2024

Draft #3491

Closed

Merge 'v0.40.0' into grarco/better-gas-interface

d0cf2ab

grarco added 2 commits July 8, 2024 17:26

Changelog #3428

378ab47

Updates gas limit in unit tests

c0ab314

grarco added 2 commits July 9, 2024 11:33

Fixes gas payment in masp integration tests

f2f34a4

Updates gas in ibc e2e test

fb81246

grarco and others added 4 commits July 9, 2024 14:06

Adds conversion from WholeGas to GasLimit

71340cb

Returns WholeGas from dry_run_tx

4f0b027

update Hermes

13f0f3f

Increases masp gas limit in genesis files and removes custom setup in…

826c2fc

… integration tests

brentstone reviewed Jul 9, 2024

View reviewed changes

crates/sdk/src/tx.rs Outdated Show resolved Hide resolved

brentstone reviewed Jul 9, 2024

View reviewed changes

Logs tx used gas

209da0c

brentstone added the ready for draft label Jul 10, 2024

brentstone added the merged to draft label Jul 11, 2024

grarco mentioned this pull request Jul 12, 2024

Update gas for storage occupation #3510

Merged

2 tasks

brentstone merged commit a575ba1 into main Jul 24, 2024
32 of 38 checks passed

brentstone deleted the grarco/better-gas-interface branch July 24, 2024 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve gas interface #3428

Improve gas interface #3428

grarco commented Jun 19, 2024 •

edited

Loading

codecov bot commented Jun 19, 2024 •

edited

Loading

grarco commented Jun 20, 2024

grarco commented Jun 20, 2024

yito88 commented Jun 20, 2024

grarco commented Jun 20, 2024 •

edited

Loading

brentstone commented Jul 5, 2024

grarco commented Jul 8, 2024

yito88 commented Jul 9, 2024

grarco commented Jul 9, 2024 •

edited

Loading

brentstone Jul 9, 2024

grarco Jul 9, 2024

grarco Jul 10, 2024 •

edited

Loading

		write!(f, "Transaction is valid. Gas used: {}", self.gas_used)
		write!(f, "Transaction is valid.")

Improve gas interface #3428

Improve gas interface #3428

Conversation

grarco commented Jun 19, 2024 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

codecov bot commented Jun 19, 2024 • edited Loading

Codecov Report

grarco commented Jun 20, 2024

grarco commented Jun 20, 2024

yito88 commented Jun 20, 2024

grarco commented Jun 20, 2024 • edited Loading

brentstone commented Jul 5, 2024

grarco commented Jul 8, 2024

yito88 commented Jul 9, 2024

grarco commented Jul 9, 2024 • edited Loading

brentstone Jul 9, 2024

Choose a reason for hiding this comment

grarco Jul 9, 2024

Choose a reason for hiding this comment

grarco Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

grarco commented Jun 19, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov bot commented Jun 19, 2024 •

edited

Loading

grarco commented Jun 20, 2024 •

edited

Loading

grarco commented Jul 9, 2024 •

edited

Loading

grarco Jul 10, 2024 •

edited

Loading