Validate Concurrent Messages + Update BankSend #76
Conversation
| {AccessType: AccessType_WRITE, Identifier: "a/b/c/d/e"}: true, | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Note that we're not checking for extra access ops defined, I think it's harder to check here since the path will not always result in the same set of access ops (e.g if account already exists vs if bank sends needs to create one)
I think this is fine for now as that's less f a priority and may just result in less concurrency but it's safer.
| err error | ||
| ) | ||
|
|
||
| msgCtx, msgMsCache := app.cacheTxContext(ctx, []byte{}) |
There was a problem hiding this comment.
just to make sure i understand, the reason we take another cache here is so that we can discard the events later?
There was a problem hiding this comment.
The messages should not be committed in general as it's not being returned in the Tx result, but this branch allows us to make sure that the events in the CacheKv store only reflects the access ops for this message, so we can compare it per message, since that's how access ops are defined
store/cachekv/memiterator.go
Outdated
| } | ||
|
|
||
| value := mi.Iterator.Value() | ||
| mi.eventManager.EmitResourceAccessReadEvent("iterator", key, value) |
There was a problem hiding this comment.
Not for the validation but I found it to be really useful for debugging missing Access Ops as most of the time the keys are not in a readable format.
We're also not committing the events returned from this store so I thought it would be fine to leave it here
types/accesscontrol/validation.go
Outdated
|
|
||
| // ValidateAccessOperations compares a list of events and a predefined list of access operations and determines if all the | ||
| // events that occurred are represented in the accessOperations | ||
| func ValidateAccessOperations(accessOps []AccessOperation, events []abci.Event) map[Comparator]bool { |
There was a problem hiding this comment.
i might have missed it but where do we compare resource type? it doesn't seem like we are consolidating all those different KV resource types
There was a problem hiding this comment.
Addressed in the follow up commits, passing a store key, and defining a mappign of the storekey to the resource types and prefixes to do the matching
BrandonWeng
force-pushed
the
bweng-tx-validation-testing
branch
from
3b24775 to
1e59ce1
Compare
| return comparators | ||
| } | ||
|
|
||
| func (c *Comparator) DependencyMatch(accessOp AccessOperation, prefix []byte) bool { |
There was a problem hiding this comment.
in the process of adding more unit tests for this method
BrandonWeng
force-pushed
the
bweng-tx-validation-testing
branch
from
81adcfe to
7540df1
Compare
4 participants
Describe your changes and provide context
Will also need to add validation for ante handlers
Testing performed to validate your change
Testing part of this PR:
sei-protocol/sei-chain#345