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

move cip 21 into last call #209

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Conversation

cmwaters
Copy link
Contributor

No description provided.

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

The reference implementation still needs to be filled out

cips/cip-21.md Show resolved Hide resolved
Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

  • Add reference implementation
  • Add last call deadline
    +1 to @rootulp's review

bytes data = 2;
uint32 share_version = 3;
uint32 namespace_version = 4;
+ // Signer is sdk.AccAddress that paid for this blob. This field is optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] can this feature be used with feegrant? If yes, and we have:

  1. celestiaA agrees to pay gas for all message for celestiaB
  2. celestiaB submits an authored blob and uses gas from celestiaA.
  3. Whose address goes in the signer field?

If it's celestiaB then should we stop using the term "that paid for this blob" and instead use something like:

// Signer is sdk.AccAddress that submitted this blob.

Copy link
Member

Choose a reason for hiding this comment

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

good catch!

Copy link
Member

@jcstein jcstein Oct 17, 2024

Choose a reason for hiding this comment

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

After reading through this CIP again recently, I'm a little confused how this would handle blobs submitted with feegrant as described in this thread. cc: @cmwaters as well for review

bytes data = 2;
uint32 share_version = 3;
uint32 namespace_version = 4;
+ // Signer is sdk.AccAddress that paid for this blob. This field is optional
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ // Signer is sdk.AccAddress that paid for this blob. This field is optional
+ // Signer is sdk.AccAddress that signed this blob. This field is optional

Copy link
Member

@jcstein jcstein Oct 1, 2024

Choose a reason for hiding this comment

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

So even though the fee granter paid for the blob to be included on-chain, the MsgPayForBlobs submitter (celestia1rgj3z5rfydj3xulhm6glqr0zs7j8rr9rfh6ekw) is the authored blob "signer".

@rootulp - So in this case shouldn't it be // Signer is sdk.AccAddress that signed this blob?

@rootulp
Copy link
Collaborator

rootulp commented Oct 1, 2024

Callum is OOO, maybe we should merge this as-is and we can edit it after the fact if that comment about "paid for" vs "submitted" is confusing for anyone else.

cc: @jcstein thoughts?

@jcstein
Copy link
Member

jcstein commented Oct 1, 2024

I think we can implement this feedback after it's in Last Call. However, here's some food for thought:

IIRC, the fee Granter/Payer account does not show any messages when blobs are posted. So If I had blob X with Grantee/Signer A and Granter/Payer B, it would only show on chain for Grantee/Signer A

Account Tx Message on explorer?
A: Grantee/Signer PayForBlobs Yes
B: Granter/Payer PayForBlobs from "A: Grantee/Signer" No, see transactions and messages tabs
B: Granter/Payer FeeGrant Yes

Accounts used in demo:
A: Grantee/Signer: celestia1rgj3z5rfydj3xulhm6glqr0zs7j8rr9rfh6ekw on Mocha
B: Granter/Payer: celestia1sung806mz9r4lwdpaw6usk7hcw42ku7kyun44w on Mocha

@rootulp
Copy link
Collaborator

rootulp commented Oct 1, 2024

Thanks for the links! That seems like expected behavior because the fee granter didn't submit a MsgPayForBlobs in this example. So even though the fee granter paid for the blob to be included on-chain, the MsgPayForBlobs submitter (celestia1rgj3z5rfydj3xulhm6glqr0zs7j8rr9rfh6ekw) is the authored blob "signer".

@rootulp rootulp dismissed jcstein’s stale review October 1, 2024 15:18

Callum addressed blocking feedback.

cips/cip-21.md Show resolved Hide resolved
@rootulp rootulp merged commit 1dda7cc into celestiaorg:main Oct 1, 2024
2 checks passed
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.

3 participants