CIP-64 / CIP-66 compatible `TransactionArgs` #123

ezdac · 2024-05-08T10:34:11Z

Fixes #109

Add CIP-66 transaction type

This introduces the CeloDenominatedTx type, which will when implemented allow for the fee-currency to be denominated in cel2 native token.

Make the `TransactionArgs` CIP-64/CIP-66 compatible

The TransactionArgs are used in some API endpoints to fill incomplete fields of passed in transactions and then
convert this internally to an EVM-message or Transaction. This PR adds code that distinguishes some combination of passed in fields for the TransactionArgs between CIP-64 and CIP-66 transactions in order to create the concrete internal transaction type. The filling of missing required fields now considers wether the transaction is Celo-denominated or not.
Additionally, the new MaxFeeInFeeCurrency field is passed along to the internal transaction representations.

Co-authored-by: bandu

Closes #109

The fee-currency conversion pre-London (legacy-tx) was unneccessary, since we don't allow celo transactions here. Additionally some sanity-checks regarding Celo related fields were missing

karlb

Looks fine to me. Just some minor comments.

Will this allow creating CIP-64 txs from the JS console, or does that require additional changes?

karlb · 2024-06-11T14:17:06Z

core/types/transaction.go

 	feeCurrency() *common.Address
+	maxFeeInFeeCurrency() *big.Int


IIRC @piersy suggested that we can avoid extending TxData by checking the tx type and casting to a specific tx type whenever we want to use a tx-type-specific attribute. I think that can work, but for consistency it should either be done for feeCurrency too or not at all.
I think going with the current approach is fine for now. I just wanted to bring up the point to see if there are any strong opionions or other thoughs about it.

I generally don't like the concept of the TxData interface exposing the union of all struct fields.
If I would contribute to upstream, I would vote to keep the changes of this PR as is for the purpose of consistency though.
When considering minimizing the diff and stronger segregation of Celo specific features for our fork I tend to lean towards the suggestion of @piersy.

So do you suggest implementing this in this PR?

No, let's keep it as it is. If we want to change it, we should do so in a separate PR.

karlb · 2024-06-11T14:23:10Z

core/types/transaction.go

@@ -628,6 +632,11 @@ func (tx *Transaction) FeeCurrency() *common.Address {
 	return copyAddressPtr(tx.inner.feeCurrency())
 }

+// MaxFeeInFeeCurrency returns the max fee in the fee_currency for celo denominated txs.


The comment feels too much like the variable name. We can add some more information here.

Suggestion:

MaxFeeInFeeCurrency is only used to guard against very quickly changing exchange rates. Txs must be discarded if MaxFeeInFeeCurrency is exceeded.

Done in dbb2c57

karlb · 2024-06-11T14:35:43Z

core/state_transition.go

 type Message struct {
-	To            *common.Address
-	From          common.Address
-	Nonce         uint64
-	Value         *big.Int
-	GasLimit      uint64
-	GasPrice      *big.Int
-	GasFeeCap     *big.Int
-	GasTipCap     *big.Int
+	To        *common.Address
+	From      common.Address
+	Nonce     uint64
+	Value     *big.Int
+	GasLimit  uint64
+	GasPrice  *big.Int
+	GasFeeCap *big.Int
+	GasTipCap *big.Int
+


This looks like an unnecessary change.

Done in dbb2c57

karlb · 2024-06-11T14:37:45Z

core/state_transition.go

@@ -213,7 +213,7 @@ func TransactionToMessage(tx *types.Transaction, s types.Signer, baseFee *big.In
 	}
 	// If baseFee provided, set gasPrice to effectiveGasPrice.
 	if baseFee != nil {
-		if msg.FeeCurrency != nil {
+		if tx.Type() == types.CeloDynamicFeeTxType {


Is this change safe for the case where tx.Type() == types.CeloDynamicFeeTxType but msg.FeeCurrency == nil? Unfortunately, we didn't forbid nil currencies for CIP-64.

Yes, the ConvertGoldToCurrency function just returns the passed in value in that case (here baseFee, which has the same effect as omitting the if case).

karlb · 2024-06-11T14:39:31Z

internal/ethapi/transaction_args.go

+		gasPrice            *big.Int
+		gasFeeCap           *big.Int
+		gasTipCap           *big.Int
+		blobFeeCap          *big.Int
+		maxFeeInFeeCurrency *big.Int


Adding a // Celo specific comment in between will reduce the diff size because it breaks the formatter's alignment into two parts. Not sure if that is overall better in this case.

Done in dbb2c57

ezdac · 2024-06-12T08:35:56Z

Will this allow creating CIP-64 txs from the JS console, or does that require additional changes?

I'm not sure what JS console you mean, but in general, if you add an additional argument feeCurrency to calls like eth_fillTransaction and eth_signTransaction (local node with keystore) you should now get back a CIP-64 transaction with filled in defaults (see the added test).

However it seems that most client libraries construct their transactions by potentially making individual calls to eth_gasPrice etc, then rlp-encoding and signing locally and using eth_sendRawTransaction:
#109 (comment)

karlb · 2024-06-12T08:50:02Z

I'm not sure what JS console you mean

I meant to one you get with geth --dev console

Fixes #109 Add CIP-66 transaction type This introduces the CeloDenominatedTx type, which will when implemented allow for the fee-currency to be denominated in cel2 native token. Make the TransactionArgs CIP-64/CIP-66 compatible The TransactionArgs are used in some API endpoints to fill incomplete fields of passed in transactions and then convert this internally to an EVM-message or Transaction. This PR adds code that distinguishes some combination of passed in fields for the TransactionArgs between CIP-64 and CIP-66 transactions in order to create the concrete internal transaction type. The filling of missing required fields now considers wether the transaction is Celo-denominated or not. Additionally, the new MaxFeeInFeeCurrency field is passed along to the internal transaction representations Included commits: * Format comments in TransactionArgs fields * Add CIP-66 tx type (Celo denominated) Co-authored-by: bandu * Fix nil-deref in CIP-66 Transaction getter * Fix required RLP field for fee-currency in CIP66 * Fix initialize MaxFeePerFeeCurrency value upon copy * Convert TransactionArgs to CIP-64/66 transaction Closes #109 * Add MaxFeeInFeeCurrency to EVM Message * Fix CIP-64/66 related sanity checks The fee-currency conversion pre-London (legacy-tx) was unneccessary, since we don't allow celo transactions here. Additionally some sanity-checks regarding Celo related fields were missing * Add tests for CIP-64/66 compatible TransactionArgs * Fix call-arg naming for currency conversion * Fix comments and formatting

aaronmgdr · 2024-06-18T14:19:12Z

@karlb can we talk about the eth_fillTransaction is it true that if i send params like
{feeCurrency: "0x....", value: 0, data: '0x...'} it will give back
{feeCurrency: "0x....", value: 0, data: '0x...', maxFeePerGas: '0x', maxPriorityFeePerGas: '0x', maxFeeInFeeCurrency: '0x', gas: '0x' }

Fixes #109 Add CIP-66 transaction type This introduces the CeloDenominatedTx type, which will when implemented allow for the fee-currency to be denominated in cel2 native token. Make the TransactionArgs CIP-64/CIP-66 compatible The TransactionArgs are used in some API endpoints to fill incomplete fields of passed in transactions and then convert this internally to an EVM-message or Transaction. This PR adds code that distinguishes some combination of passed in fields for the TransactionArgs between CIP-64 and CIP-66 transactions in order to create the concrete internal transaction type. The filling of missing required fields now considers wether the transaction is Celo-denominated or not. Additionally, the new MaxFeeInFeeCurrency field is passed along to the internal transaction representations Included commits: * Format comments in TransactionArgs fields * Add CIP-66 tx type (Celo denominated) Co-authored-by: bandu * Fix nil-deref in CIP-66 Transaction getter * Fix required RLP field for fee-currency in CIP66 * Fix initialize MaxFeePerFeeCurrency value upon copy * Convert TransactionArgs to CIP-64/66 transaction Closes #109 * Add MaxFeeInFeeCurrency to EVM Message * Fix CIP-64/66 related sanity checks The fee-currency conversion pre-London (legacy-tx) was unneccessary, since we don't allow celo transactions here. Additionally some sanity-checks regarding Celo related fields were missing * Add tests for CIP-64/66 compatible TransactionArgs * Fix call-arg naming for currency conversion * Fix comments and formatting

ezdac force-pushed the ezdac/issue-109 branch 3 times, most recently from a314ec1 to 51b4d8e Compare May 8, 2024 15:15

ezdac changed the base branch from celo4 to ezdac/fix-estimate-gas May 14, 2024 09:52

ezdac force-pushed the ezdac/issue-109 branch from 51b4d8e to 44db2fc Compare May 14, 2024 12:09

ezdac force-pushed the ezdac/fix-estimate-gas branch from dc37292 to 947fc1c Compare May 14, 2024 15:45

Base automatically changed from ezdac/fix-estimate-gas to celo4 May 15, 2024 09:17

ezdac force-pushed the ezdac/issue-109 branch 3 times, most recently from b11f374 to 9998929 Compare May 16, 2024 11:39

ezdac changed the base branch from celo4 to karlb/celo5-3 May 16, 2024 11:40

ezdac force-pushed the ezdac/issue-109 branch from 9998929 to d14a9f1 Compare May 16, 2024 12:13

palango force-pushed the karlb/celo5-3 branch from 37df44b to 9ed0e13 Compare May 17, 2024 10:33

ezdac changed the base branch from karlb/celo5-3 to celo5 May 22, 2024 11:03

ezdac force-pushed the ezdac/issue-109 branch from d14a9f1 to 2cf3ac9 Compare May 22, 2024 11:09

ezdac mentioned this pull request May 22, 2024

Ensure TransactionArgs works with cip-64 and cip-66 #109

Closed

ezdac self-assigned this Jun 5, 2024

ezdac added 9 commits June 11, 2024 12:36

Format comments in TransactionArgs fields

d434cce

Add CIP-66 tx type (Celo denominated)

577d152

Co-authored-by: bandu

Fix nil-deref in CIP-66 Transaction getter

526563d

Fix required RLP field for fee-currency in CIP66

8e46539

Fix initialize MaxFeePerFeeCurrency value upon copy

9f238e6

Convert TransactionArgs to CIP-64/66 transaction

9d0aa80

Closes #109

Add MaxFeeInFeeCurrency to EVM Message

815cb7c

Fix CIP-64/66 related sanity checks

7a87ae6

The fee-currency conversion pre-London (legacy-tx) was unneccessary, since we don't allow celo transactions here. Additionally some sanity-checks regarding Celo related fields were missing

Add tests for CIP-64/66 compatible TransactionArgs

3689a77

ezdac force-pushed the ezdac/issue-109 branch from 2cf3ac9 to 3689a77 Compare June 11, 2024 11:40

Fix call-arg naming for currency conversion

27a328d

ezdac marked this pull request as ready for review June 11, 2024 13:09

ezdac requested a review from karlb June 11, 2024 13:14

karlb reviewed Jun 11, 2024

View reviewed changes

Fix comments and formatting

dbb2c57

karlb approved these changes Jun 12, 2024

View reviewed changes

ezdac merged commit 404e6b2 into celo5 Jun 12, 2024
7 checks passed

ezdac deleted the ezdac/issue-109 branch June 12, 2024 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CIP-64 / CIP-66 compatible `TransactionArgs` #123

CIP-64 / CIP-66 compatible `TransactionArgs` #123

ezdac commented May 8, 2024 •

edited

Loading

karlb left a comment

karlb Jun 11, 2024

ezdac Jun 11, 2024 •

edited

Loading

karlb Jun 12, 2024

karlb Jun 11, 2024

ezdac Jun 11, 2024

karlb Jun 11, 2024

ezdac Jun 11, 2024

karlb Jun 11, 2024

ezdac Jun 11, 2024

karlb Jun 11, 2024

ezdac Jun 11, 2024

ezdac commented Jun 12, 2024 •

edited

Loading

karlb commented Jun 12, 2024

aaronmgdr commented Jun 18, 2024 •

edited

Loading

CIP-64 / CIP-66 compatible TransactionArgs #123

CIP-64 / CIP-66 compatible TransactionArgs #123

Conversation

ezdac commented May 8, 2024 • edited Loading

Add CIP-66 transaction type

Make the TransactionArgs CIP-64/CIP-66 compatible

karlb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezdac Jun 11, 2024 • 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

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

ezdac commented Jun 12, 2024 • edited Loading

karlb commented Jun 12, 2024

aaronmgdr commented Jun 18, 2024 • edited Loading

CIP-64 / CIP-66 compatible `TransactionArgs` #123

CIP-64 / CIP-66 compatible `TransactionArgs` #123

ezdac commented May 8, 2024 •

edited

Loading

Make the `TransactionArgs` CIP-64/CIP-66 compatible

ezdac Jun 11, 2024 •

edited

Loading

ezdac commented Jun 12, 2024 •

edited

Loading

aaronmgdr commented Jun 18, 2024 •

edited

Loading