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

ledger: rename "internal" package to "eval" #4272

Closed
wants to merge 2 commits into from

Conversation

cce
Copy link
Contributor

@cce cce commented Jul 18, 2022

Summary

This will let third-party programs like indexer directly call StartEvaluator and EvaluatorOptions rather than have to place code in go-algorand to call by proxy, and should make it possible to remove evalindexer.go, as well as the indexerLedgerForEval and indexerLedgerConnector implementations (some of it will move to indexer).

Test Plan

Existing tests should pass — just a quick package rename.

@cce cce requested a review from jannotti July 18, 2022 19:18
@jannotti
Copy link
Contributor

jannotti commented Jul 18, 2022

I think I like this, I never understood the reasoning behind the internal package. But I want to think a bit about eval. We have two major things that we use eval for - the evaluation of txns and the evaluation of avm programs. And then we also have an apply package. Can anyone succinctly describe what this newly named package does? Should it just go back into ledger? Is it about storage and lookup? (to my mind, that would make it part of ledger) or is it about applying rules of processing (to my mind that's eval but it's also apply).

I don't care if the meaning is perfect now, I like the simplicity of a simple package rename PR (or re-absorption into ledger). Just trying to get the best possible name.

Perhaps this is the cow package?

@cce
Copy link
Contributor Author

cce commented Jul 18, 2022

Hrmm ... well, eval contains the BlockEvaluator, which uses layered storage "cow" types to model the environment used to evaluate each transaction, handling reads and recording writes made by each one. The apply package contains the implementation of all transaction logic, operating inside these layered environments, which it accesses using an implementation of the apply.Balances interface implemented by eval's "cow" storage. WDYT

@algorand algorand deleted a comment from codecov bot Jul 18, 2022
@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #4272 (b529186) into master (2a933eb) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #4272   +/-   ##
=======================================
  Coverage   55.24%   55.25%           
=======================================
  Files         395      395           
  Lines       50219    50219           
=======================================
+ Hits        27744    27749    +5     
  Misses      20093    20093           
+ Partials     2382     2377    -5     
Impacted Files Coverage Δ
ledger/eval/appcow.go 76.14% <ø> (ø)
ledger/eval/applications.go 62.50% <ø> (ø)
ledger/eval/assetcow.go 100.00% <ø> (ø)
ledger/eval/compactcert.go 77.77% <ø> (ø)
ledger/eval/cow.go 76.63% <ø> (ø)
ledger/eval/cow_creatables.go 52.17% <ø> (ø)
ledger/eval/eval.go 67.90% <ø> (ø)
ledger/eval/evalindexer.go 0.00% <ø> (ø)
ledger/eval/prefetcher/error.go 33.33% <ø> (ø)
ledger/eval/prefetcher/prefetcher.go 68.18% <ø> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@jannotti
Copy link
Contributor

I don't feel strongly. Getting it out of internal seems like a big enough win to not keep myself up about the name.

@cce cce mentioned this pull request Jul 27, 2022
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Approving to get it off my pending list. We should definitely get this out of internal

@algorandskiy
Copy link
Contributor

@winder please confirm that the Indexer does not need to import and call any go-algorand package.

@winder
Copy link
Contributor

winder commented Nov 1, 2022

@algorandskiy this is less important for Indexer today as it was in July. I think there are still good reasons to open it up. Tools like algojig might want to call these functions.

@algorandskiy
Copy link
Contributor

Thanks Will. Then maybe interested parties could refresh it.

@cce
Copy link
Contributor Author

cce commented Nov 9, 2022

Closing in favor of #4777

@cce cce closed this Nov 9, 2022
@cce cce deleted the rename-internal-eval branch November 9, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants