Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!(state/core_access): add TxConfig #3349

Merged
merged 24 commits into from
Jul 4, 2024

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Apr 30, 2024

Resolves #3346

What was done:
Implemented a new TxConfig struct

// TxConfig specifies additional options that will be applied to the Tx.
type TxConfig struct {
	// Specifies the address from the keystore that will sign transactions.
	// NOTE: Only `accountAddress` or `KeyName` should be passed.
	// accountAddress is a primary options. This means If both the address and the key are specified,
	// the address field will take priority.
	signerAddress string
	// Specifies the key from the keystore associated with an account that
	// will be used to sign transactions.
	// NOTE: This `Account` must be available in the `Keystore`.
	keyName string
	// gasPrice represents the amount to be paid per gas unit.
	// Negative gasPrice means user want us to use the minGasPrice
	// defined in the node.
	gasPrice float64
	// since gasPrice can be 0, it is necessary to understand that user explicitly set it.
	isGasPriceSet bool
	// 0 gas means users want us to calculate it for them.
	gas uint64

	// Specifies the account that will pay for the transaction.
	// Input format Bech32.
	feeGranterAddress string
}

The keyName field allows the user to use a non-default account from the Keystore to pay fees.
The granter field is moved from the state config. The user will need to specify the granter address each time he wants to submit a tx with a granter. The reason for moving the granter to the options struct is that previously user had to restart the node each time when he wanted to start/stop using grantee mode.
Tested on mocha with several accounts:

celestia blob submit 0x42690c204d39600fddd3 'gm' --key.name testKey --gas 140000
{
  "result": {
    "height": 1383824,
    "commitments": [
      "IXg+08HV5RsPF3Lle8PH+B2TUGsGUsBiseflxh6wB5E="
    ]
  }
}
celestia blob submit 0x42690c204d39600fddd3 'gm' --gas.price 0.1 --gas 1400000 --granter.address celestia14s2aeemrzw34crdh79e6wj56kjzpu4ffmvxsyx
{
  "result": {
    "height": 1383809,
    "commitments": [
      "IXg+08HV5RsPF3Lle8PH+B2TUGsGUsBiseflxh6wB5E="
    ]
  }
}
celestia state transfer celestia14s2aeemrzw34crdh79e6wj56kjzpu4ffmvxsyx 10000 --gas 100000 --gas.price 0.1
{
  "result": {
    "height": 1383817,
    "txhash": "1F38E94AFAE8AA4C4819844100803B8D7C83016DB1C02148F332FBB8E8DBC857",

List of breaking changes:

  • Added a TxConfig struct that is applied to all write transactions(transfer/submitPFB etc);
  • Renamed KeyringAccName to DefaultKeyName and KeyringBackend to DefaultBackendName in the config;
  • Removed blob.GasPrice since blob.Submit requires SubmitOptions which are type alias of TxConfig;
  • Reworked flags for tx submission through CLI;
  • Added state.Blob which is an alias for app.Blob(State Module now requires blob type from app)(Was added to avoid circular dependency between node's blob Module and node's state Module);

@vgonkivs vgonkivs added area:state Related to fetching state and state execution kind:feat Attached to feature PRs labels Apr 30, 2024
@vgonkivs vgonkivs self-assigned this Apr 30, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 59.71429% with 141 lines in your changes missing coverage. Please review.

Project coverage is 45.66%. Comparing base (2469e7a) to head (cdc22aa).
Report is 135 commits behind head on main.

Files Patch % Lines
state/core_access.go 55.37% 42 Missing and 12 partials ⚠️
nodebuilder/state/mocks/api.go 12.12% 29 Missing ⚠️
nodebuilder/state/cmd/state.go 74.60% 16 Missing ⚠️
state/tx_config.go 87.87% 5 Missing and 3 partials ⚠️
blob/service.go 0.00% 7 Missing ⚠️
nodebuilder/state/state.go 12.50% 7 Missing ⚠️
blob/helper.go 0.00% 5 Missing ⚠️
nodebuilder/blob/cmd/blob.go 50.00% 4 Missing ⚠️
nodebuilder/state/keyring.go 33.33% 3 Missing and 1 partial ⚠️
nodebuilder/blob/blob.go 0.00% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3349      +/-   ##
==========================================
+ Coverage   44.83%   45.66%   +0.82%     
==========================================
  Files         265      274       +9     
  Lines       14620    15453     +833     
==========================================
+ Hits         6555     7056     +501     
- Misses       7313     7590     +277     
- Partials      752      807      +55     

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

distractedm1nd
distractedm1nd previously approved these changes Apr 30, 2024
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me.
Just re-iterating that the most important feature/user-request is that users can submit blobs from different accounts. No other options should be added before that particular one. Having an options struct part of the JSON-RPC further more manifests that this current API is basically driven by implementation details (this is very go idiomatic). No blocker to merge this but sth I wanted to raise.

cc @renaynay

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - want to discuss the gasLim option there

Should we also update ADR 9 (API ADR)?

state/core_access.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
nodebuilder/state/flags.go Outdated Show resolved Hide resolved
nodebuilder/state/flags.go Outdated Show resolved Hide resolved
state/state.go Outdated Show resolved Hide resolved
nodebuilder/state/keyring.go Outdated Show resolved Hide resolved
nodebuilder/state/keyring.go Outdated Show resolved Hide resolved
nodebuilder/state/keyring.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs changed the title feat(state/core_accessor): add SubmitPayForBlobWithOptions endpoint feat!(state/core_accessor): add SubmitPayForBlobWithOptions endpoint May 7, 2024
@vgonkivs vgonkivs added the kind:break! Attached to breaking PRs label May 7, 2024
nodebuilder/state/flags.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
state/state.go Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for potential follow-up PR:
should we break SubmitPayForBlob on the state level to not take gasLim or fee and just use defaults? And then we can make blob module use SubmitPayForBlobWithOptions under the hood if opts are specified?

nodebuilder/state/flags.go Outdated Show resolved Hide resolved
nodebuilder/state/flags.go Outdated Show resolved Hide resolved
nodebuilder/state/keyring.go Outdated Show resolved Hide resolved
nodebuilder/testing.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to add the optional parameters to the struct in this PR as discussed above?

distractedm1nd
distractedm1nd previously approved these changes May 10, 2024
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vgonkivs
Copy link
Member Author

should we break SubmitPayForBlob on the state level to not take gasLim or fee and just use defaults? And then we can make blob module use SubmitPayForBlobWithOptions under the hood if opts are specified?

Yes, I also thought about it.

@vgonkivs vgonkivs dismissed stale reviews from renaynay, Wondertan, and ramin via 88875ff July 4, 2024 13:12
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFG

@vgonkivs vgonkivs disabled auto-merge July 4, 2024 14:08
@vgonkivs vgonkivs enabled auto-merge (squash) July 4, 2024 14:10
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@vgonkivs vgonkivs merged commit 3d26a1a into celestiaorg:main Jul 4, 2024
28 checks passed
Comment on lines +45 to +47
// gasPrice represents the amount to be paid per gas unit.
// Negative gasPrice means user want us to use the minGasPrice
// defined in the node.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: minGasPrice is a bit ambiguous because in celestia-app version >= 2, consensus nodes have a local min gas price. There is also a global network min gas price which is a governance modifiable parameter so perhaps it is worth clarifying which gas price this defaults to: the local min gas price or the network min gas price.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:state Related to fetching state and state execution kind:break! Attached to breaking PRs kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

state | nodebuilder/state: Implement TxOptions