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: Add support for avail as a DA #355

Merged
merged 15 commits into from
Jun 25, 2023
Merged

Conversation

omritoptix
Copy link
Contributor

@omritoptix omritoptix commented Jun 15, 2023

This pr aims to provide with a stable avail client to dymint.
Currently there is a tx size limitation of 15kb on avail side which makes it unusable for dymint due to (at least) 2 reasons:

  1. fast block rate can possibly lead to a gap between the sequencer and the hub which is ever growing
  2. one block can possibly be bigger then the limitation

The issues were discussed with the avail team and they are working on increasing the max tx size ASAP.
Either way we can merge it and it should be usable once they do.

PR Standards

Opening a pull request should be able to meet the following requirements


For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@omritoptix omritoptix linked an issue Jun 15, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #355 (5c4f20b) into main (5abf3ca) will decrease coverage by 3.11%.
The diff coverage is 11.01%.

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   55.39%   52.29%   -3.11%     
==========================================
  Files          86       89       +3     
  Lines       13234    14197     +963     
==========================================
+ Hits         7331     7424      +93     
- Misses       4879     5740     +861     
- Partials     1024     1033       +9     
Impacted Files Coverage Δ
config/toml.go 70.37% <ø> (ø)
testutil/mocks.go 88.57% <0.00%> (-8.31%) ⬇️
mocks/da/avail/SubstrateApiI.go 3.93% <3.93%> (ø)
da/avail/avail.go 19.83% <19.83%> (ø)
config/config.go 40.00% <50.00%> (+0.57%) ⬆️
block/manager.go 69.69% <100.00%> (+1.73%) ⬆️
config/defaults.go 100.00% <100.00%> (ø)
da/celestia/celestia.go 68.44% <100.00%> (-0.51%) ⬇️
da/registry/registry.go 100.00% <100.00%> (ø)
da/utils.go 100.00% <100.00%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@omritoptix omritoptix marked this pull request as ready for review June 15, 2023 17:25
@omritoptix omritoptix requested a review from a team as a code owner June 15, 2023 17:25
@omritoptix omritoptix assigned mtsitrin and unassigned mtsitrin Jun 18, 2023
@@ -158,6 +160,8 @@ func TestProduceOnlyAfterSynced(t *testing.T) {
<-ctx.Done()
require.Greater(t, manager.store.Height(), lastStoreHeight)
assert.Equal(t, batch.EndHeight, manager.store.Height())
// Wait until manager is done
time.Sleep(time.Second * 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

why u need it? isn't the manager stopped by the context already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is but it takes the manager time due to the other case running (so the context.Done() case doesn't get a context switch immediately but only after a few seconds).
the tests kept failing so this fixed it.
prob it's bit dirty. if you have a better idea..

config/toml.go Outdated
# da_config = "{\"base_url\": \"http://127.0.0.1:26659\", \"timeout\": 60000000000, \"fee\":20000, \"gas_limit\": 20000000, \"namespace_id\":\"000000000000ffff\"}"
da_config = "{{ .DAConfig }}"
# Avail config example:
# da_config = “{\“seed\“: \“MNEMONIC\“, \“api_url\“: \“wss://kate.avail.tools/ws\“, \“app_id\“: 0, \“tip\“:10}“,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like there are curly quotes (vs straight quotes)

mtsitrin
mtsitrin previously approved these changes Jun 21, 2023
@omritoptix omritoptix merged commit 1a683ca into main Jun 25, 2023
@omritoptix omritoptix deleted the omritoptix/49-add-polygon-avail branch June 25, 2023 09:51
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.

Add a polygon avail DA client
2 participants