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): enable fee granting #3304

Merged
merged 11 commits into from
Apr 16, 2024
Merged

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Apr 8, 2024

Resolves #3256

Tested on mocha:

  1. Run funded light node and grant unlimited fee to the newly created FN:
    celestia state grant-fee celestia12psg90lwvxdktnqerr2p5mxuw2v3je497uv6tj 2000 1000000 --node.store ~/.celestia-light-mocha-4/

  2. Run FN and submitted blob:
    celestia blob submit 0x42690c204d39600fddd3 'gm' --node.store ~/.celestia-full-mocha-4/
    Result: blob was successfully submitted:

{
  "result": {
    "height": 1562820,
    "commitments": [
      "IXg+08HV5RsPF3Lle8PH+B2TUGsGUsBiseflxh6wB5E="
    ]
  }
}
  1. Run revoke-fee cmd on the LN:
    celestia state revoke-grant-fee celestia12psg90lwvxdktnqerr2p5mxuw2v3je497uv6tj 2000 1000000 --node.store ~/.celestia-light-mocha-4/

  2. Submitted blob from the FN and got an error back:

{
  "result": ": insufficient funds"
}
  1. Run grant fee with a very small limit:
    celestia state grant-fee celestia12psg90lwvxdktnqerr2p5mxuw2v3je497uv6tj 2000 1000000 --amount 10 --node.store ~/.celestia-light-mocha-4/

  2. Submitting blobs now returns an error:

{
  "result": ": fee limit exceeded"
}

@vgonkivs vgonkivs added area:state Related to fetching state and state execution kind:feat Attached to feature PRs kind:break! Attached to breaking PRs labels Apr 8, 2024
@vgonkivs vgonkivs requested a review from Bidon15 April 8, 2024 14:23
@vgonkivs vgonkivs self-assigned this Apr 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 17.03704% with 112 lines in your changes are missing coverage. Please review.

Project coverage is 44.81%. Comparing base (2469e7a) to head (9f5f453).
Report is 24 commits behind head on main.

Files Patch % Lines
nodebuilder/state/cmd/state.go 18.36% 40 Missing ⚠️
state/core_access.go 14.63% 31 Missing and 4 partials ⚠️
nodebuilder/state/mocks/api.go 0.00% 18 Missing ⚠️
nodebuilder/state/state.go 0.00% 4 Missing ⚠️
nodebuilder/state/stub.go 0.00% 4 Missing ⚠️
cmd/util.go 0.00% 3 Missing ⚠️
nodebuilder/state/core.go 0.00% 3 Missing ⚠️
nodebuilder/state/flags.go 62.50% 3 Missing ⚠️
nodebuilder/state/module.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3304      +/-   ##
==========================================
- Coverage   44.83%   44.81%   -0.02%     
==========================================
  Files         265      272       +7     
  Lines       14620    15061     +441     
==========================================
+ Hits         6555     6750     +195     
- Misses       7313     7545     +232     
- Partials      752      766      +14     

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

@Bidon15
Copy link
Member

Bidon15 commented Apr 9, 2024

Can you try this approach and see if this works? 🙏
#3284

@vgonkivs vgonkivs changed the title feat!(state): enable fee granting feat(state): enable fee granting Apr 9, 2024
@vgonkivs vgonkivs removed the kind:break! Attached to breaking PRs label Apr 9, 2024
@vgonkivs
Copy link
Member Author

vgonkivs commented Apr 9, 2024

Hey @Bidon15

Please find all info below:

celestia-appd tx feegrant grant \
  celestia1ly7vmh26hffvau52ktnxff557t4u8thh0gwtyc celestia12psg90lwvxdktnqerr2p5mxuw2v3je497uv6tj \
   --home ~/.celestia-light-mocha-4/keys \
   --node public-celestia-mocha4-consensus.numia.xyz:26657 \
   --spend-limit 1000000utia \
   --allowed-messages "/cosmos.bank.v1beta1.MsgSend,/celestia.blob.v1.MsgPayForBlobs" \
   --chain-id mocha-4 \
   --keyring-backend test \
   --fees 20000utia \
   --broadcast-mode block \
   --yes

https://mocha-4.celenium.io/tx/4a317410cf3a0ccc86169a1774d796fc3f37a47f109f7a00664ea155fcc86e9b

https://mocha-4.celenium.io/tx/165865c66fa785bae7b9e73b21abe20b1c0cff97ad1a80dbde0b3a6ee29f4003

{"id":368437257,"height":1570138,"time":"2024-04-09T14:07:12.797236Z","position":2,"tx_id":3696947,"type":"coin_spent","data":{"amount":"176utia","spender":"celestia1ly7vmh26hffvau52ktnxff557t4u8thh0gwtyc"}}

Снимок экрана 2024-04-09 в 17 07 34

nodebuilder/state/cmd/state.go Outdated Show resolved Hide resolved
nodebuilder/state/cmd/state.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Logically, these changes are sound and I only have a few adjacent comments.

While I was reviewing this, I was thinking about the broader picture. Will node API eventually support all transactions that can be submitted to the Celestia state machine or only a subset that is specifically relevant to rollup nodes? If we're taking a minimalist stance I don't see why the grant-fee and revoke-grant-fee cli needs to be supported. These already exist in the app cli so do we really need to duplicate it here.

My naive implementation of this would have been to add a field feeGranter in the coreAccessor struct that can be set in the constructor and then simply in every SubmitPayForBlob, check if it is set and then set it in the transaction that goes to the Signer - that way light nodes are now compliant with the fee granter protocol

nodebuilder/state/cmd/state.go Outdated Show resolved Hide resolved
nodebuilder/state/cmd/state.go Show resolved Hide resolved
@Wondertan
Copy link
Member

Wondertan commented Apr 10, 2024

Will node API eventually support all transactions that can be submitted to the Celestia state machine?

I would say so, considering merge and API unification efforts.

We can start piece by piece as use cases come.

Thoughts or objections?

@cmwaters
Copy link
Contributor

Thoughts or objections?

I like the general principle of it doesn't matter which node you talk to, they all have the same API

nodebuilder/state/flags.go Outdated Show resolved Hide resolved
nodebuilder/state/module.go Outdated Show resolved Hide resolved
state/core_access.go Show resolved Hide resolved
state/core_access.go Show resolved Hide resolved
state/core_access.go Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
vgonkivs and others added 2 commits April 15, 2024 18:12
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
nodebuilder/state/flags.go Outdated Show resolved Hide resolved
nodebuilder/state/module.go Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM.

nodebuilder/state/cmd/state.go Show resolved Hide resolved
nodebuilder/state/cmd/state.go Show resolved Hide resolved
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:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Signer support for fee grant module for blob submission
6 participants