refactor: create senders based on escrow account version #624
Conversation
gusinacio
changed the title
gusinacio
force-pushed
the
gustavo/horizon-escrow
branch
5 times, most recently
from
f2a47e1 to
cdb15fa
Compare
| @@ -95,12 +95,33 @@ pub async fn escrow_accounts( | |||
| reject_thawing_signers: bool, | |||
| ) -> Result<EscrowAccountsWatcher, anyhow::Error> { | |||
| indexer_watcher::new_watcher(interval, move || { | |||
| get_escrow_accounts(escrow_subgraph, indexer_address, reject_thawing_signers) | |||
| get_escrow_accounts_v1(escrow_subgraph, indexer_address, reject_thawing_signers) | |||
| }) | |||
| .await | |||
| } | |||
There was a problem hiding this comment.
I think this function should be renamed to escrow_accounts_v1, it can make it a bit confusing? since in every other spot you are making the difference in the names between v1 and v2 and this could give the idea is generic for both versions
There was a problem hiding this comment.
You are absolutely right, I just didn't want to change this because it would have tons of other changes (since it's also used in service).
I could I do this renaming in a follow-up PR, unless you think it's mandatory.
There was a problem hiding this comment.
Maybe just create an issue so that this is not forgotten?
| ) -> anyhow::Result<EscrowAccounts> { | ||
| Ok(EscrowAccounts::new(HashMap::new(), HashMap::new())) | ||
| } | ||
|
|
There was a problem hiding this comment.
Add TODO: Change query for temporary implementation of subgraph handling both v1 and V2
There was a problem hiding this comment.
There's already a TODO on top of the function. Do you want me to change the message?
There was a problem hiding this comment.
I just tried to mean that the current comment kind of only sounds for v2 and we might need to modify also v1 query later
There was a problem hiding this comment.
At the moment you change the provided schema old queries will break so you will quickly realize that you need to change the query.
On the other hand, this won't break and that's why there's a TODO
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
gusinacio
force-pushed
the
gustavo/horizon-escrow
branch
from
4051582 to
da09924
Compare
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
|
Nice one @gusinacio taking inspiration from #625 👍 |
Pull Request Test Coverage Report for Build 13292922750Warning: 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
💛 - Coveralls |
This PR aims to enable creation of two kinds of SenderAccounts:
Actor names now have the type as a prefix and they pass different versions of the escrow_account_watcher, depending on the type (since there'll be different queries).
Actors receive their enum SenderType so they can route and query properly the corresponding db tables.
For future PRs:
unimplementedparts, specially the sender state (allow, deny)Extras: