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

[CHIA-710] Add the concept of 'action scopes' #18124

Merged
merged 11 commits into from
Jun 17, 2024
Merged

Conversation

Quexington
Copy link
Contributor

@Quexington Quexington commented Jun 6, 2024

This PR creates a new utility called ActionScope. The motivation for this originated in the wallet, but this utility could be more broadly applicable. The purpose is to provide a standard way for multiple functions/coroutines to access and modify some shared state as part of a single transaction.

When an action scope is created the scope is passed from function to coroutine etc. and if a logical process wishes to update or read from the state, it must "check out" the state and return it when it is done. No other process can check out the state while it is checked out by another. Processes may also register callbacks to be run when the action scope is about to close. Right before the action scope is closed, these callbacks run and then the state is deleted. Other layers on top can choose to commit that state or whatever else if they so wish.

The wallet motivation more specifically is to provide a single hub for all of the state changes that will happen as part of creating a transaction. Right now, every function is responsible for just making side effects at will and some higher process is responsible for making sure these side effects don't happen prematurely and get rolled back if something goes wrong. Every step along the way needs to make sure the information is sent up and down properly so that everything works properly. It's quite a burden. With action scopes, everything just modifies this temporary state and then the thing that made the action scope can decide what to do with everything when it closes. In addition, every function that is called can see the whole context it is operating in rather than just what its caller has told it. This will be important for vault singletons later on.

This may sound very similar to the DBWrapper functionality. You would be correct in thinking that as that is what action scopes use as a back end. It's not clear we want to always use an in memory sqlite DB but it's the fastest path to getting all of these features. In any case, the backend is a protocol that can be switched out at a later date, even if only in some areas.

@Quexington Quexington added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Jun 6, 2024
@Quexington Quexington changed the title Add the concept of 'action scopes' [CHIA-714] Add the concept of 'action scopes' Jun 6, 2024
@Quexington Quexington changed the title [CHIA-714] Add the concept of 'action scopes' [CHIA-710] Add the concept of 'action scopes' Jun 6, 2024
@Chia-Network Chia-Network deleted a comment from github-actions bot Jun 6, 2024
@Quexington Quexington marked this pull request as ready for review June 6, 2024 17:09
@Quexington Quexington requested a review from a team as a code owner June 6, 2024 17:09
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

What affect does an error in a callback have on a transaction? I think it's complex because what if the first callback goes through (and this is where side effects would expect to get implemented?) and the second fails? How do we recover?

Relatedly, there's the option of concurrent callbacks (good if they are independent, bad if they depend on order which maybe is questionable anyways if they are added by code in unrelated places which is unaware of the other code) or just callback order in general, and also the idea of exception groups since we have multiple things to do.

Whatever should happen, there should be a test for nested .use() managers. I believe that is allowed presently due to the way the two-layers-down DBWrapper2 works.

chia/_tests/util/test_action_scope.py Show resolved Hide resolved
chia/_tests/util/test_action_scope.py Outdated Show resolved Hide resolved
chia/_tests/util/test_action_scope.py Outdated Show resolved Hide resolved
chia/_tests/util/test_action_scope.py Outdated Show resolved Hide resolved
chia/util/action_scope.py Outdated Show resolved Hide resolved
chia/util/action_scope.py Outdated Show resolved Hide resolved
chia/util/action_scope.py Outdated Show resolved Hide resolved
chia/util/action_scope.py Outdated Show resolved Hide resolved
chia/util/action_scope.py Outdated Show resolved Hide resolved
chia/util/action_scope.py Outdated Show resolved Hide resolved
@Quexington
Copy link
Contributor Author

I killed memos because implementing this upstream I didn't use them (I will eventually) and they can just be part of side effects. Separating them out was pointless. I also removed the ability to have a list of callbacks due to the concerns around ordering. There's no good solution for that, so instead, I changed the paradigm so that people have to know about what other callbacks they are potentially interfering with (because there's exactly one place where they can all live).

Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

chatted with quex, we need a better understanding around what is being updated, do we accidentally have multiple rows? etc.

@Quexington Quexington closed this Jun 11, 2024
@Quexington Quexington reopened this Jun 11, 2024
@Quexington Quexington closed this Jun 13, 2024
@Quexington Quexington reopened this Jun 13, 2024
@Quexington Quexington closed this Jun 17, 2024
@Quexington Quexington reopened this Jun 17, 2024
Copy link
Contributor

File Coverage Missing Lines
chia/_tests/util/test_action_scope.py 98.8% lines 133
chia/util/action_scope.py 97.8% lines 37, 109
Total Missing Coverage
172 lines 3 lines 98%

Copy link

coveralls-official bot commented Jun 17, 2024

Pull Request Test Coverage Report for Build 9549203077

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 169 of 172 (98.26%) changed or added relevant lines in 2 files are covered.
  • 28 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.004%) to 90.874%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/_tests/util/test_action_scope.py 82 83 98.8%
chia/util/action_scope.py 87 89 97.75%
Files with Coverage Reduction New Missed Lines %
chia/introducer/introducer.py 1 78.26%
chia/timelord/timelord.py 2 73.67%
chia/server/node_discovery.py 2 78.9%
chia/data_layer/data_layer_wallet.py 2 91.93%
chia/full_node/weight_proof.py 4 90.35%
chia/introducer/introducer_api.py 5 78.26%
chia/wallet/wallet_node.py 12 88.07%
Totals Coverage Status
Change from base Build 9524346852: 0.004%
Covered Lines: 100184
Relevant Lines: 110187

💛 - Coveralls

@markelrod
Copy link
Contributor

coverage exceptions are fine

@cmmarslender cmmarslender merged commit a36c0b8 into main Jun 17, 2024
1361 of 1416 checks passed
@cmmarslender cmmarslender deleted the quex.action_scopes branch June 17, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants