ledger: Implement LookupAgreement #3046
Conversation
algorandskiy
changed the title
| data, err := l.accts.LookupWithRewards(rnd, addr) | ||
| if err != nil { | ||
| return basics.OnlineAccountData{}, err | ||
| } |
There was a problem hiding this comment.
tiny nit, would you want to have this call Ledger.Lookup() for now just to remove the duplication, or maybe it doesn't matter because it will be refactored soon
There was a problem hiding this comment.
I'm hoping that in the context of this PR, we'd better get off to the account update level - and stop there.
That would ensure that when we make the next changes, these won't "leak" outside the account updates.
There was a problem hiding this comment.
I just meant that this new LookupAgreement() is mostly a copy-paste of the Lookup() function above, whether it was less repetitive to call it directly for now.. not a big issue though
Codecov Report
@@ Coverage Diff @@
## master #3046 +/- ##
==========================================
+ Coverage 43.65% 43.67% +0.01%
==========================================
Files 391 391
Lines 86779 86792 +13
==========================================
+ Hits 37887 37909 +22
+ Misses 42864 42859 -5
+ Partials 6028 6024 -4
Continue to review full report at Codecov.
|
|
Removed |
| func (u AccountData) VotingStake() MicroAlgos { | ||
| // OnlineAccountData returns subset of AccountData as OnlineAccountData data structure. | ||
| // Account is expected to be Online otherwise its stake is cleared out | ||
| func (u AccountData) OnlineAccountData() OnlineAccountData { |
There was a problem hiding this comment.
The code here suggest that you could have a situation where you have a non-online account which has any of the VoteID/SelectionID/VoteFirstValid/VoteLastValid/VoteKeyDilution non-zero, which isn't correct.
If Status is online, then all these fields are valid. Otherwise, they are all empty.
There was a problem hiding this comment.
I also thought about it but decided to keep the same as before. But I agree, returning empty if not online looks correct.
There was a problem hiding this comment.
Looks good — assuming in the current code these AccountData fields are currently always zero when Status is non-online, the agreement side will behave the same either way.
There was a problem hiding this comment.
looks good to me, but I'd like @cce to take a look as well.
## Summary Use LookupAgreement in AlgorandFullNode.VotingKeys() Post #3046 fixes.
## Summary 1. Add a new `OnlineAccountData` data type 2. Implement `LookupAgreement` ## Test Plan Fixed existed tests: added `LookupAgreement` to mocked ledgers
## Summary Use LookupAgreement in AlgorandFullNode.VotingKeys() Post algorand#3046 fixes.
Summary
OnlineAccountDatadata typeLookupAgreementTest Plan
Fixed existed tests: added
LookupAgreementto mocked ledgers