TX counter spike #608
TX counter spike #608
Conversation
Codecov Report
@@ Coverage Diff @@
## master #608 +/- ##
==========================================
- Coverage 60.05% 60.01% -0.04%
==========================================
Files 45 48 +3
Lines 5212 5257 +45
==========================================
+ Hits 3130 3155 +25
- Misses 1860 1879 +19
- Partials 222 223 +1
|
|
I don't really understand why it's necessary for contracts to be aware of their in-block/in-tx position. 😅 |
To clarify my comment above, we talked in a private chat with @webmaster128, @ethanfrey & @reuvenpo about a way for contract calls within the same tx to have different I mistakenly thought that this PR is about a "call counter" and not a "tx counter", and that's what my above comment refers to. Sorry for the misunderstanding. |
|
Now unblocked with #617 |
| } | ||
| currentHeight := ctx.BlockHeight() | ||
| store := ctx.KVStore(a.storeKey) | ||
| var txCounter uint64 = 0 |
There was a problem hiding this comment.
This logic seems correct but is a little confusing to follow. Maybe there is no easier way.
load the store, if there is data:
- if same height stored, use the stored value
- if old height stored, update stored height and set value to 0
then save the height and next counter
This may be the best way to do it, just found the flow a little confusing (maybe I am just spoiled by Rust where if can return values)
There was a problem hiding this comment.
🤔 I tried to follow the advice but there is also the case where the store is empty that requires a 0 init value. I am not sure if it would get easier by adding else blocks. Instead I have added more code comments.
| } | ||
|
|
||
| func encodeHeightCounter(height int64, counter uint64) []byte { | ||
| return append(sdk.Uint64ToBigEndian(uint64(height)), sdk.Uint64ToBigEndian(counter)...) |
There was a problem hiding this comment.
super efficient.
I was expecting a protobuf type here, but this works as well.
maybe a protobuf type would be more maintainable, if we ever wanted to add a field, but I don't think that will ever be the case
There was a problem hiding this comment.
I would use a uint32 type for the counter now but otherwise leave it as it is. We can always refactor as needed. Depending on the case we another ante handler may be a better option to keep things separated. 🤷
| // TXCounter return tx counter value | ||
| // Will default to `0` when not set for example in external queries or simulations | ||
| func TXCounter(ctx sdk.Context) uint64 { | ||
| val, ok := ctx.Value(contextKeyTXCount).(uint64) |
There was a problem hiding this comment.
what value is returned if it is never set?
I think it may be useful to differentiate between "we know it is index 0" and "no info there".
We have both on the Rust side (Option<TransactionInfo>). Maybe return (uint64, found)?
There was a problem hiding this comment.
Nice, as far as I can tell.
Can we make the counter uint32 consistently? This is what I used on the contract side:
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
pub struct TransactionInfo {
/// The position of this transaction in the block. The first
/// transaction has index 0.
///
/// This allows you to get a unique transaction indentifier in this chain
/// using the pair (`env.block.height`, `env.transaction.index`).
///
pub index: u32,
}
|
Closed in favour of #621 |
🚧 SPIKE: do not merge
See #601
In this spike I have added an ante handler that counts the TX in a block and passes this down to the wasmvm Env.
0.Not included:
Does this work for you @reuvenpo @assafmo ?