Implement simple transaction memos #1280
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1280 +/- ##
===========================================
+ Coverage 65.26% 65.28% +0.01%
===========================================
Files 102 102
Lines 5519 5533 +14
===========================================
+ Hits 3602 3612 +10
- Misses 1708 1712 +4
Partials 209 209 |
|
Why don't transaction memos have fees? Is it because the implementation details would be complicated? Intuitively it seems as though they should to me, since its taking long term storage on the chain. (Unless there is an in-protocol way to specify "Prune memos after blocks") |
Memos are stored in history, but not state (so they can be discarded by non-archiving nodes). |
|
I think we need to add to the godoc somewhere, that the reason memo gas is significantly cheaper than normal gas per byte is that we plan to have them rapidly pruned once pruning has been implemented. |
cwgoes
changed the title
Agreed, let's do that in #1289 though. |
x/auth/ante.go
Outdated
|
|
||
| if len(memo) > maxMemoCharacters { | ||
| return ctx, | ||
| sdk.ErrMemoTooLarge(fmt.Sprintf("maximum %d characters but was %d characters", maxMemoCharacters, len(memo))).Result(), |
There was a problem hiding this comment.
Consider changing to something more verbose, e.g. "maximum number of characters is %d, but received %d characters"
x/auth/ante_test.go
Outdated
| // tx has enough gas | ||
| fee = NewStdFee(1000, sdk.NewCoin("atom", 0)) | ||
| tx = newTestTx(ctx, msg, privs, accnums, seqs, fee) | ||
| checkValidTx(t, anteHandler, ctx, tx) |
There was a problem hiding this comment.
This doesn't test the inclusion of memo when it is of a correct size, and enough fees is provided. Such a test needs to show that the memo is included in the state.
There was a problem hiding this comment.
Added a test for inclusion of memo less than maximum size. State isn't really a property of the ante handler, that would make sense to test elsewhere.
| barAcc = executeGetAccount(t, fmt.Sprintf("gaiacli account %v %v", barCech, flags)) | ||
| assert.Equal(t, int64(30), barAcc.GetCoins().AmountOf("steak").Int64()) | ||
| fooAcc = executeGetAccount(t, fmt.Sprintf("gaiacli account %v %v", fooCech, flags)) | ||
| assert.Equal(t, int64(20), fooAcc.GetCoins().AmountOf("steak").Int64()) |
There was a problem hiding this comment.
Not sure if possible through just cli commands, but ideally this should check that the memo field is in history. Currently this is just checking that inclusion of the memo field doesn't break sends.
There was a problem hiding this comment.
Checking memo in history would be nice but is inconvenient to do through CLI tests, ideally in the future we'll have better Tendermint integration tests - #1313.
x/auth/mock/simulate_block.go
Outdated
| @@ -45,12 +45,12 @@ func GenTx(msg sdk.Msg, accnums []int64, seq []int64, priv ...crypto.PrivKeyEd25 | |||
| for i, p := range priv { | |||
| sigs[i] = auth.StdSignature{ | |||
| PubKey: p.PubKey(), | |||
| Signature: p.Sign(auth.StdSignBytes(chainID, accnums, seq, fee, msg)), | |||
| Signature: p.Sign(auth.StdSignBytes(chainID, accnums, seq, fee, msg, "")), | |||
There was a problem hiding this comment.
Should this ideally just be checking on an empty memo? I think we may want to include some non-empty message, just to check that memo inclusion doesn't break stuff.
There was a problem hiding this comment.
Changed to include a non-empty message.
Expected use case: simple, cheap way to associate a transaction with a reference to an external resource (exchange account, coffee payment, sender identity).
Memos are not indexed or query-able, but they are included in the signature. Memos are per-transaction, not per-message. Memos cost a bit of gas and are capped at 100 characters (presently).
Closes #904
Ref #669
AltBytesto memo