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

Avail DA Adapter in Rollkit #6

Merged
merged 26 commits into from
Sep 5, 2023
Merged

Avail DA Adapter in Rollkit #6

merged 26 commits into from
Sep 5, 2023

Conversation

chandiniv1
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@aterentic-ethernal aterentic-ethernal left a comment

Choose a reason for hiding this comment

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

From my side all looks good, comments are mostly cosmetics. If I'm not wrong, there are few things to consider in the future, or maybe to put comments in the code:

  • KeyPair if from seed, I guess that should be more configurable
  • Does the rollkit retries if block is not yet available, thats not explicit in adapter code?
  • Is there a case when multiple blocks are fetched in one call
  • Does Message field needs concanated extrinsics

Hope that helps :)

da/avail/avail.go Show resolved Hide resolved
da/avail/avail.go Show resolved Hide resolved
da/avail/avail.go Outdated Show resolved Hide resolved
da/avail/avail.go Outdated Show resolved Hide resolved
da/avail/avail.go Outdated Show resolved Hide resolved
BaseHeader: types.BaseHeader{
Height: blockNumber,
},
AggregatorsHash: make([]byte, 32),
Copy link

Choose a reason for hiding this comment

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

don't we need to send any calculated hash here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@anilcse ,Actually celestia block structure and avail block structure are different . I manually assigned the appdata(got from avail endpoints) into txs part of the celestia block with the help of helper functions. So we are not sure about other fields like AggregatorHash,IntermediateStateRoots etc.For now I'll remove those empty fields or If you have any idea regarding how to get those fields, please suggest.Thank you

Copy link

Choose a reason for hiding this comment

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

@murthyvitwit any idea about these?

da/avail/avail.go Outdated Show resolved Hide resolved
da/avail/datasubmit/submitdata.go Outdated Show resolved Hide resolved
return err
}

keyringPair, err := signature.KeyringPairFromSecret(seed, 42)
Copy link

Choose a reason for hiding this comment

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

Not sure if this is a secure way of doing it. Did you check other cosmos implementations on how this is being handled?

return err
}

key, err := types.CreateStorageKey(meta, "System", "Account", keyringPair.PublicKey)
Copy link

Choose a reason for hiding this comment

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

Do we need to create storate key everytime we are trying to submit the data?

@murthyvitwit murthyvitwit merged commit e04e0a0 into main Sep 5, 2023
14 of 16 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.

5 participants