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(app)!: add authz to the state-machine #1540

Merged
merged 1 commit into from
Mar 28, 2023
Merged

feat(app)!: add authz to the state-machine #1540

merged 1 commit into from
Mar 28, 2023

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented Mar 24, 2023

Overview

TODOs(notes to myself mainly)

  • figure out if we need/want to add authz to our own module (to e.g. make it possible to pay for blobs or qgb txs via another account) I think the answer here is: yes, we should properly integrate authz with the blobs module. Main reason is that it can enable (e.g. rollkit) sequnecers with a known public key to delegate submitting pfbs to another hot-key, while clients can still validate against the known pub-key. The feegrant module only allows fees to be payed by the grantee the txs would be submitted by a different account/key though. This can happen in a follow-up PR though (needs an issue) and should be accompanied with a brief ADR for the rationale/motivation. Thanks to @zmanian for walking through potential user stories with me.
  • double-check if I did not miss anything
  • talk to team wrt to docs

ref: #1530

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@cmwaters
Copy link
Contributor

I think the answer here is: yes, we should properly integrate authz with the blobs module.

What is this scope of this? Currently there is a GenericAuthorization which could be set to allow an account to execute all arbitrary PayForBlob messages. We could introduce some authorizations which permit the grantee to only execute PayForBlob messages for a certain namespace and/or under a maximum blob size. Does that sound right? We can do this as a separate PR

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.

LGTM - I don't think you've missed any places where authz needs to be declared

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

optionally, we could add a test case to the standard sdk smoke test, but I'll defer to the author as its purely bonus. also added #1546

LGTM

@liamsi liamsi marked this pull request as ready for review March 28, 2023 12:37
@liamsi liamsi merged commit debf804 into main Mar 28, 2023
@liamsi liamsi deleted the liamsi/add-authz branch March 28, 2023 12:38
@gitpoap-bot
Copy link

gitpoap-bot bot commented Mar 28, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 Celestia Contributor:

GitPOAP: 2023 Celestia Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@liamsi liamsi mentioned this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants