Refactor how messages are processed #129

evan-forbes · 2021-10-01T03:07:09Z

Description

This PR follows what was outlined in this #33 (comment) , where we utilize a more standard sdk.Tx instead of creating a custom one for SignedTxPayForMessage, which let's us plug into the cosmos sdk in a less hacky/more traditional way.

closes: #28, #33
blocked by #127

codecov-commenter · 2021-10-05T23:30:17Z

Codecov Report

Merging #129 (cb9810a) into master (18e69b5) will increase coverage by 12.42%.
The diff coverage is 52.64%.

@@             Coverage Diff             @@
##           master     #129       +/-   ##
===========================================
+ Coverage   19.58%   32.01%   +12.42%     
===========================================
  Files          15       13        -2     
  Lines        2783     2271      -512     
===========================================
+ Hits          545      727      +182     
+ Misses       2211     1493      -718     
- Partials       27       51       +24

Impacted Files	Coverage Δ
app/export.go	`0.00% <ø> (ø)`
x/payment/types/genesis.pb.go	`1.69% <0.00%> (+0.02%)`	⬆️
x/payment/types/keys.go	`0.00% <ø> (ø)`
x/payment/types/tx.pb.gw.go	`0.00% <0.00%> (ø)`
x/payment/types/tx.pb.go	`16.49% <16.94%> (+15.45%)`	⬆️
x/payment/types/payformessage.go	`57.24% <30.43%> (-12.94%)`	⬇️
x/payment/types/builder.go	`42.60% <50.00%> (ø)`
app/abci.go	`61.01% <57.14%> (-4.46%)`	⬇️
x/payment/types/wirepayformessage.go	`68.46% <68.46%> (ø)`
app/app.go	`82.94% <83.63%> (+0.92%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d061ef0...cb9810a. Read the comment docs.

evan-forbes

This is a pretty hefty change for this repo, and is essentially a rewrite of how the app handles messages. Sorry ahead of time for cramming all this into a single PR, but the larger important changes are all dependent upon each other.

Before this PR, the app had a separate Tx type called TxSignedTransactionDataPayForMessage, whose purpose is to pay for messages submitted to celestia. This has been replaced by a standard authsigning.Tx. There also used to be a message that TxSignedTransactionDataPayForMessage wrapped, called MsgSignedTransactionDataPayForMessage. This message has been simplified into MsgPayForMessage.
https://github.com/celestiaorg/lazyledger-app/blob/4eda3ddb92071fb7e29d2233bc397b448cd384b9/proto/payment/tx.proto#L38-L46
Before, we had to stick things like nonce and fees into the message portion, now we keep sequence, gas limit, and gas price in the standard sdk authsigning.Tx. This allows us to plug into the sdk in a much more conventional way.

A similar treatment has been applied to what used to be called MsgWirePayForMessage, portions of the struct have been moved to a standard sdk authsigning.Tx, and only the minimal fields are left. This was first suggested by Ismail in this comment.
https://github.com/celestiaorg/lazyledger-app/blob/4eda3ddb92071fb7e29d2233bc397b448cd384b9/proto/payment/tx.proto#L18-L26

Most of the rest of the changes introduced in this PR surround these two changes.

There’s also a few overdue code quality changes.

evan-forbes · 2021-10-05T23:58:17Z

app/abci.go

-		coreMsg, signedTx, err := app.processMsg(msg)
+		coreMsg, unsignedPFM, sig, err := types.ProcessWirePayForMessage(wireMsg, app.SquareSize())
 		if err != nil {
 			continue
 		}

+		signedTx, err := types.BuildPayForMessageTxFromWireTx(authTx, app.txConfig.NewTxBuilder(), sig, unsignedPFM)
+		if err != nil {
+			// todo(evan): log all of these potential errors
+			continue
+		}


Here we are building a new MsgPayForMessage into a standard sdk.Tx by using one of the signatures included in MsgWirePayForMessage, but using the same fees, gas price, and sequence of the transaction that contains the MsgWirePayForMessage.

This differs from the previous approach, which created a an entirely new transaction that was reliant upon a separate fee struct for fees.

this is closer to how the sdk expects transactions to be created/used

evan-forbes · 2021-10-06T03:41:47Z

x/payment/types/codec.go

+// makeEncodingConfig is copied here so that we don't have to have an
+//  import cycle. if possible, use cosmoscmd.MakeEncodingConfig
+func makeEncodingConfig() cosmoscmd.EncodingConfig {
+	amino := codec.NewLegacyAmino()
+	interfaceRegistry := codectypes.NewInterfaceRegistry()
+	RegisterInterfaces(interfaceRegistry)
+	marshaler := codec.NewProtoCodec(interfaceRegistry)
+	txCfg := tx.NewTxConfig(marshaler, tx.DefaultSignModes)
+
+	return cosmoscmd.EncodingConfig{
+		InterfaceRegistry: interfaceRegistry,
+		Marshaler:         marshaler,
+		TxConfig:          txCfg,
+		Amino:             amino,
+	}
+}


The newer version of the sdk registers proto interfaces for us using the app package’s ModuleBasics struct when we create encoding configs. This works for most use cases, but we need to handle building new transactions manually in the types package, so this had to be copied here.

evan-forbes · 2021-10-06T03:43:14Z

x/payment/keeper/keeper.go

+func (k Keeper) PayForMessage(goCtx context.Context, msg *types.MsgPayForMessage) (*types.MsgPayForMessageResponse, error) {
+	// @liamsi here's where we can use the bank keeper to burn fees
 	return &types.MsgPayForMessageResponse{}, nil
 }


@liamsi I think spoke about this briefly during the sprint planning session. If we end up being able to move away from deferred execution, and could get burning fraud provable, then I think we would be able to burn fees here

TBH, I think if we adopt fee burning, we should adopt it for all Txs and not just for PayForMessage. So it would still be in the auth module (or a modification of that) instead IMO. I think @marbar3778 had a diff laying around somewhere that implemented general fee-burning. We can just use that instead.

Also, I really think we should talk with the SDK and tendermint team about immediate execution. If there is a strong point to adopt immediate exec by default in the baseapp (which I think there is), than we should support that implementation directly upstream first IMO.

https://github.com/marbar3778/fee/blob/main/app/feeParam.go

i never finished the burning but its a simple change in the ante handler

I see, that would be a separate module. I think that is also good. I just do not think we should implement feeburning only for PayForMessage (if at all) but if it should be applied to all Txs.

evan-forbes · 2021-10-06T03:47:30Z

x/payment/keeper/keeper.go

-	TokenDenomination = "token"
+	TokenDenomination = "tia"


Idk what the token denomination will actually be and am not attached to this name whatsoever, just thought we wouldn't use plain old "token".

I like tia.

evan-forbes · 2021-10-06T03:49:53Z

proto/payment/tx.proto

+//  Multiple versions are signed and included, each version creates a commitment for a 
+// specific square size.
+message MsgPayForMessage {
+    string signer = 1;


adding the signer or "creator" as the bech32 address of the key that signed is a new default in starport, so I figured we could keep it. Happy to remove.

evan-forbes · 2021-10-06T03:50:48Z

proto/payment/tx.proto

 message MsgWirePayForMessage {
-  TransactionFee fee = 1;
-  uint64 nonce = 2;
-  bytes message_name_space_id = 3; // assume this is 8 bytes!
-  uint64 message_size = 4;
-  bytes message = 5;
+  string signer = 1;
+  bytes message_name_space_id = 2; // assume this is 8 bytes!
+  uint64 message_size = 3;
+  bytes message = 4;
  repeated ShareCommitAndSignature message_share_commitment = 6 [(gogoproto.nullable) = false];
-  bytes public_key = 7; 
 }


simplified wire message

liamsi

First pass looks good. So finally the fees get actually payed by deleting a bunch of code 👏🏼

x/payment/types/wirepayformessage.go

liamsi · 2021-10-06T21:47:53Z

proto/payment/tx.proto

  }

-// WirePayForMessage describes the format of data that is sent over the wire for
+// MsgWirePayForMessage describes the format of data that is sent over the wire for
 // each PayForMessage
 message MsgWirePayForMessage {


Is the naming inspired by the specs? What is with the Msg*Message? cc @adlerjohn

Yeah, the Msg prefix is just an sdk suggested syntax thing (ie MsgCreatePool), and the second Message is referring to a core.Message like the specs do. It is annoying, but I think it also makes sense. 🤷

x/payment/types/payformessage.go

liamsi · 2021-10-06T21:52:49Z

x/payment/types/payformessage.go

@@ -264,7 +136,7 @@ func CreateCommitment(k uint64, namespace, message []byte) ([]byte, error) {
 	// create the commits by pushing each leaf set onto an nmt
 	subTreeRoots := make([][]byte, len(leafSets))
 	for i, set := range leafSets {
-		// create the nmt
+		// create the nmt todo(evan) use nmt wrapper


So the nmt wrapper will be used here and in the celestia-node. And potenially to verify inclusion in other clients. We should move that code into another repo too probably. (cc @renaynay)

Yes celestiaorg/celestia-node#111

x/payment/types/wirepayformessage.go

liamsi · 2021-10-06T21:56:00Z

x/payment/types/wirepayformessage.go

+
+	// ensure that a reserved namespace is not used
+	if bytes.Compare(msg.GetMessageNameSpaceId(), consts.MaxReservedNamespace) < 1 {
+		return errors.New("message is not valid: uses a reserved namesapce ID")


We need to document this pretty well somewhere for people building rollups.

tac0turtle · 2021-10-08T06:37:37Z

it seems there are some unrelated changes in this PR. Are they coming from upstream?

Could updates be pulled into a separate PR? it will be easier to track commits and revert items if nessecary.

tac0turtle · 2021-10-08T06:38:25Z

app/export.go

@@ -4,7 +4,7 @@ import (
 	"encoding/json"
 	"log"

-	tmproto "github.com/celestiaorg/celestia-core/proto/tendermint/types"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"


The gomod name is now back to tendermint/tendermint: https://github.com/celestiaorg/celestia-core/blob/c2df2c4ce0942b8e974fd2b530355f2a58139926/go.mod#L1

And pointing to the fork is now done via a replace directive here: https://github.com/celestiaorg/lazyledger-app/blob/215166853ebaccc266fd3755599d3ca5450b5136/go.mod#L129

Anything wrong with that?

tac0turtle · 2021-10-08T06:40:21Z

app/abci.go

+	abci "github.com/tendermint/tendermint/abci/types"
+	"github.com/tendermint/tendermint/pkg/consts"
+	core "github.com/tendermint/tendermint/proto/tendermint/types"


tac0turtle · 2021-10-08T06:46:04Z

So finally the fees get actually payed by deleting a bunch of code

I cant find where this changed. I dont see the ante handler being touched, so i could be looking in the wrong place

app/abci.go

app/app.go

app/export.go

app/genesis.go

x/payment/genesis.go

x/payment/client/cli/tx_wire_pay_for_message.go

x/payment/types/payformessage.go

x/payment/types/wirepayformessage.go

liamsi · 2021-10-08T09:03:21Z

I cant find where this changed. I dont see the ante handler being touched, so i could be looking in the wrong place

@marbar3778 the idea was to simply use the default underlying fee deduction mechanism assuming that the included messages' gas simply gets metered by bytes. I thought the auth module (or its ante handlers) would provide this.

liamsi · 2021-10-08T09:54:53Z

brief summary with chat with marko:

without custom fee logic, the default fee applies (which is locally set and could be zero and which is fine and part of the goal of this PR)
if custom logic is necessary just add it to the AnteHandler

Let's just make sure that if the base-fee is non-zero, we pay more if the blob/msg the payformessage commits to gets larger. Every other aspect around fees is not important right now!

liamsi · 2021-10-08T15:59:45Z

Let's just make sure that if the base-fee is non-zero, we pay more if the blob/msg the payformessage commits to gets larger.

OK, this is apparently out of scope for this PR and paying for the blob will be handled in another PR.

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

#132

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

evan-forbes · 2021-10-08T21:40:49Z

it seems there are some unrelated changes in this PR. Are they coming from upstream?

Yeah, sorry we merged the branch that this PR was based off of #127 last evening and I didn't rebase this branch until today. Hopefully this is better. There are still a few minor code quality changes and some additions to tests that were orthogonal to this PR and definitely should have been in a separate PR.

So finally the fees get actually payed by deleting a bunch of code

I cant find where this changed. I dont see the ante handler being touched, so i could be looking in the wrong place

This PR doesn't change how the fees are getting handled. I miscommunicated that by including #59 and #48 in this PR's description! I think I was going to handle those issues when this was a draft PR by burning the fees in the payment keeper, but then removed that later as it is not the best mechanism and this PR was already too big. This PR only changes the way we handle messages (comment) . Fees for MsgPayForMessage are still handled like a normal sdk.Msg, but the rollup data (core.Message) is not yet paid for.

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

…r-app into evan/refactor-wpfm

evan-forbes · 2021-10-12T15:05:16Z

@liamsi @adlerjohn is there anything else needed to merge this PR? it's not blocking anything AFAIK, so no rush or anything

adlerjohn · 2021-10-12T17:59:08Z

@evan-forbes

is there anything else needed to merge this PR?

Yes, this one! #129 (comment)

liamsi

I think this looks good! Looking forward to the followup PR that further deducts fees for the included message-blob too.

adlerjohn

evan-forbes self-assigned this Oct 1, 2021

evan-forbes changed the title ~~Evan/refactor wpfm~~ Refactor the payment module Oct 1, 2021

evan-forbes changed the base branch from master to evan/regenerate-app October 5, 2021 14:17

evan-forbes commented Oct 6, 2021

View reviewed changes

evan-forbes marked this pull request as ready for review October 6, 2021 03:53

evan-forbes requested review from Bidon15, adlerjohn and liamsi October 6, 2021 03:54

evan-forbes changed the title ~~Refactor the payment module~~ Refactor how messages are processed Oct 6, 2021

liamsi reviewed Oct 6, 2021

View reviewed changes

evan-forbes mentioned this pull request Oct 6, 2021

regeneration of app #127

Merged

Base automatically changed from evan/regenerate-app to master October 7, 2021 13:18

tac0turtle reviewed Oct 8, 2021

View reviewed changes

adlerjohn reviewed Oct 8, 2021

View reviewed changes

evan-forbes added 6 commits October 8, 2021 13:46

delete old generated app

5adcc74

update proto to use tendermint instead of celestia-core

d4f715d

regenerate and adjust application

3fde1ac

use cosmoscmd instead of copying everything over

aba62ee

regenerate and adjust module boilerplate

7b8d57f

copy over and adjust client code

465354a

evan-forbes and others added 6 commits October 8, 2021 13:49

clean up todos and final touches

b823270

docs

5340112

improve error message

74e5518

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

improved docs

211a724

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

add punctuation

ed9d1af

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

change file name after rebase

43b1303

evan-forbes force-pushed the evan/refactor-wpfm branch from 2151668 to 43b1303 Compare October 8, 2021 19:40

evan-forbes and others added 3 commits October 8, 2021 14:49

remove comment

18bbd57

fix proto

b5e777b

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

remove generated iota

57b8dfd

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

evan-forbes mentioned this pull request Oct 8, 2021

Fix WirePayForMessage transaction generation when using cli #132

Closed

evan-forbes and others added 4 commits October 8, 2021 15:59

better docs for SignShareCommitment

ca10bb7

use more idiomatic comparisons

5958359

#132

better docs for SignShareCommitments

d4981ed

keep comment

703aaec

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

evan-forbes and others added 3 commits October 8, 2021 16:46

move function over to test

f11c413

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

Merge branch 'evan/refactor-wpfm' of github.com:celestiaorg/lazyledge…

7faab6f

…r-app into evan/refactor-wpfm

change celestia-core tag to more idiomatic tag

677d924

actually use nsLen for the error

cb9810a

liamsi approved these changes Oct 12, 2021

View reviewed changes

adlerjohn approved these changes Oct 13, 2021

View reviewed changes

evan-forbes merged commit accbe90 into master Oct 13, 2021

evan-forbes mentioned this pull request Nov 4, 2021

Refactor to stop using the txbuilder more than once #144

Closed

evan-forbes mentioned this pull request Dec 3, 2021

Remove the Nonce field in MsgWirePayForMessage #33

Closed

evan-forbes mentioned this pull request Jan 4, 2022

Update cli and add cli integration test #171

Merged

2 tasks

rootulp deleted the evan/refactor-wpfm branch September 21, 2022 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor how messages are processed #129

Refactor how messages are processed #129

evan-forbes commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 5, 2021 •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes Oct 5, 2021

evan-forbes Oct 6, 2021

evan-forbes Oct 6, 2021 •

edited

Loading

liamsi Oct 6, 2021 •

edited

Loading

tac0turtle Oct 8, 2021

liamsi Oct 8, 2021 •

edited

Loading

evan-forbes Oct 6, 2021

liamsi Oct 6, 2021

adlerjohn Oct 8, 2021

evan-forbes Oct 6, 2021

evan-forbes Oct 6, 2021

liamsi left a comment

liamsi Oct 6, 2021

evan-forbes Oct 7, 2021

liamsi Oct 6, 2021

renaynay Oct 8, 2021

liamsi Oct 6, 2021

tac0turtle commented Oct 8, 2021 •

edited

Loading

tac0turtle Oct 8, 2021

liamsi Oct 8, 2021

tac0turtle Oct 8, 2021

tac0turtle commented Oct 8, 2021

liamsi commented Oct 8, 2021

liamsi commented Oct 8, 2021

liamsi commented Oct 8, 2021

evan-forbes commented Oct 8, 2021 •

edited

Loading

evan-forbes commented Oct 12, 2021 •

edited

Loading

adlerjohn commented Oct 12, 2021

liamsi left a comment

adlerjohn left a comment

Refactor how messages are processed #129

Refactor how messages are processed #129

Conversation

evan-forbes commented Oct 1, 2021 • edited Loading

Description

codecov-commenter commented Oct 5, 2021 • edited Loading

Codecov Report

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

liamsi Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 8, 2021 • 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

liamsi left a 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

Choose a reason for hiding this comment

tac0turtle commented Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Oct 8, 2021

liamsi commented Oct 8, 2021

liamsi commented Oct 8, 2021

liamsi commented Oct 8, 2021

evan-forbes commented Oct 8, 2021 • edited Loading

evan-forbes commented Oct 12, 2021 • edited Loading

adlerjohn commented Oct 12, 2021

liamsi left a comment

Choose a reason for hiding this comment

adlerjohn left a comment

Choose a reason for hiding this comment

evan-forbes commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 5, 2021 •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes Oct 6, 2021 •

edited

Loading

liamsi Oct 6, 2021 •

edited

Loading

liamsi Oct 8, 2021 •

edited

Loading

tac0turtle commented Oct 8, 2021 •

edited

Loading

evan-forbes commented Oct 8, 2021 •

edited

Loading

evan-forbes commented Oct 12, 2021 •

edited

Loading