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

Feature/evm reader multi dapp #512

Merged
merged 9 commits into from
Jul 19, 2024
Merged

Conversation

fmoura
Copy link
Contributor

@fmoura fmoura commented Jul 9, 2024

Closes #332

To manually test EVM Reader Standalone first you need to build Devnet. In a terminal, type:

make docker-build-deps

then start the dependencies

go run ./cmd/cartesi-rollups-cli/ run-deps

setup the database. Open a new terminal and type:

go run ./cmd/cartesi-rollups-cli/ db upgrade

then, setup envrironment variables

source ./setup_env.sh 

and finally start EVM Reader

go run ./cmd/cartesi-rollups-evm-reader/ --verbose

Now to "interact" with it, add aplications

go run ./cmd/cartesi-rollups-cli/ app add -a 0x2E663fe9aE92275242406A185AA4fC8174339D3E  -n 16

-a parameter sets the application address and -n is Inputbox Deployment block number

To send inputs use something like:

go run ./cmd/cartesi-rollups-cli/ send --address-book ./addresses.json --payload "0xdeadbeef"

where addresses.json is a addresses.json file, where you can set the application contract you want

@fmoura fmoura added this to the 2.0.0 milestone Jul 9, 2024
@fmoura fmoura self-assigned this Jul 9, 2024
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch 3 times, most recently from 2bbef0b to 984437c Compare July 9, 2024 17:37
@fmoura fmoura added the #feat:evm-reader Feature: evm reader label Jul 9, 2024
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch 2 times, most recently from 0b20cf4 to 9839466 Compare July 9, 2024 19:46
@fmoura fmoura changed the title Feature/evm reader muilti dapp Feature/evm reader multi dapp Jul 9, 2024
@GMKrieger GMKrieger force-pushed the feature/multi-dapp-repository branch from c44553c to ed637c6 Compare July 9, 2024 20:07
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch from 9839466 to caee21b Compare July 9, 2024 20:12
@GMKrieger GMKrieger force-pushed the feature/multi-dapp-repository branch from ed637c6 to 217c97d Compare July 9, 2024 20:25
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch 2 times, most recently from cf24743 to 426abc3 Compare July 9, 2024 20:53
@GMKrieger GMKrieger force-pushed the feature/multi-dapp-repository branch 2 times, most recently from 20ad909 to 8c701de Compare July 10, 2024 11:12
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch from 426abc3 to f9c9a73 Compare July 10, 2024 13:01
@GMKrieger GMKrieger force-pushed the feature/multi-dapp-repository branch 2 times, most recently from a376638 to 23825d2 Compare July 10, 2024 14:53
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch from f9c9a73 to f62bf70 Compare July 10, 2024 15:13
@GMKrieger GMKrieger force-pushed the feature/multi-dapp-repository branch 5 times, most recently from d9f780e to 62bd20e Compare July 10, 2024 18:25
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch 3 times, most recently from 4ebb51e to 70ce3b9 Compare July 11, 2024 04:23
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch 3 times, most recently from ad1b09b to b7bbd22 Compare July 18, 2024 19:34
internal/evmreader/evmreader.go Show resolved Hide resolved
internal/evmreader/evmreader.go Outdated Show resolved Hide resolved
internal/evmreader/evmreader.go Outdated Show resolved Hide resolved
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch 2 times, most recently from 26946c6 to 5fa5fc6 Compare July 18, 2024 20:12
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch from 022a767 to 213981b Compare July 19, 2024 00:00
GMKrieger
GMKrieger previously approved these changes Jul 19, 2024
@fmoura fmoura force-pushed the feature/evm-reader-muilti-dapp branch from a4d6e2e to e7f2563 Compare July 19, 2024 14:14
@fmoura fmoura merged commit e7f2563 into next/2.0 Jul 19, 2024
6 checks passed
@fmoura fmoura deleted the feature/evm-reader-muilti-dapp branch July 19, 2024 15:25
Copy link
Collaborator

@marcelstanley marcelstanley left a comment

Choose a reason for hiding this comment

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

Approved with some suggestions to be addressed later on.

Run: run,
}

func run(cmd *cobra.Command, args []string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may need to improve this command.
In the sequence below, there's:

  • an upgrade
  • check for version 2
  • check for version 3
rollups-node$ go run ./cmd/cartesi-rollups-cli/ db upgrade
Database Schema successfully Updated. Current version is 2
rollups-node$
rollups-node$ go run ./cmd/cartesi-rollups-cli/ db check-version 2
Database Schema is at the correct version: 2
rollups-node$
rollups-node$ go run ./cmd/cartesi-rollups-cli/ db check-version 3
Database Schema is at the correct version: 2
rollups-node$

Also, message Database Schema successfully Updated. Current version is 2 should be similar to what cobra.CheckErr(err) responds (Error: No valid database schema found).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check-version do not receive arguments. It only check is the DB is at the expected version.

var (
// Should be overridden during the final release build with ldflags
// to contain the actual version number
buildVersion = "devel"
Copy link
Collaborator

Choose a reason for hiding this comment

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

buildVersion should be held elsewhere in a common place that could be shared by all binaries we provide. Se also these places

Comment on lines +134 to +152
// Validate Schema
err := startup.ValidateSchema(c.PostgresEndpoint.Value)
if err != nil {
slog.Error("EVM Reader exited with an error", "error", err)
os.Exit(1)
}

database, err := repository.Connect(ctx, c.PostgresEndpoint.Value)
if err != nil {
slog.Error("EVM Reader couldn't connect to the database", "error", err)
os.Exit(1)
}
defer database.Close()

_, err = startup.SetupNodePersistentConfig(ctx, database, c)
if err != nil {
slog.Error("EVM Reader couldn't connect to the database", "error", err)
os.Exit(1)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could generalized to be used in all necessary commands, including cartesi-rollups-node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created the "startup" package for that. This can indeed further improved, making all "node binaries" similar

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could go one step further and perform all these steps in on shot there, then.

@@ -0,0 +1,17 @@
## cartesi-rollups-cli app
Copy link
Collaborator

@marcelstanley marcelstanley Jul 19, 2024

Choose a reason for hiding this comment

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

Perhaps we could add an HTML disclaimer to all generated MD files (via gen-docs) warning they were generated like we do with the automatically generated code.

internal/evmreader/evmreader.go Show resolved Hide resolved
for i := uint64(0); i <= maxRetries; i++ {
lastValue, lastErr = fn(args)
if lastErr == nil {
return lastValue, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are missing an INFO log for a successful execution.

InputBox = inputbox.InputBox
)

// InputBox Wrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Improve docs.

Comment on lines +125 to +151
// Service tests
func (s *EvmReaderSuite) TestItStopsWhenContextIsCanceled() {
ctx, cancel := context.WithCancel(s.ctx)
ready := make(chan struct{}, 1)
errChannel := make(chan error, 1)
go func() {
errChannel <- s.evmReader.Run(ctx, ready)
}()
cancel()

err := <-errChannel
s.Require().Equal(context.Canceled, err, "stopped for the wrong reason")
}

func (s *EvmReaderSuite) TestItEventuallyBecomesReady() {
ready := make(chan struct{}, 1)
errChannel := make(chan error, 1)
go func() {
errChannel <- s.evmReader.Run(s.ctx, ready)
}()

select {
case <-ready:
case err := <-errChannel:
s.FailNow("unexpected failure", err)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did this but, on hindsight, I think we should generalize these tests and move them to https://github.com/cartesi/rollups-node/tree/next/2.0/internal/services

s.client.AssertNumberOfCalls(s.T(), "SubscribeNewHead", 1)
}

func (s *EvmReaderSuite) TestItReadsInputsFromNewBlocks() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add tests covering more than one application at a time.

Comment on lines +188 to +201
currentMostRecentFinalizedHeader, err := r.fetchMostRecentHeader(
ctx,
r.config.DefaultBlock,
)
if err != nil {
slog.Error("Error fetching most recent block",
"last default block",
r.config.DefaultBlock,
"error",
err)
continue
}
currentMostRecentFinalizedBlockNumber := currentMostRecentFinalizedHeader.Number.Uint64()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this whole block be moved outside the loop for groupedApps (BTW, appGroup would be better) to retrieve currentMostRecentFinalizedBlockNumber only once and share it between apps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#feat:evm-reader Feature: evm reader
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants