Fix reward amount and add test for reward construction #26
Conversation
rewards/src/follower/client.rs
Outdated
| @@ -18,6 +19,35 @@ pub struct FollowerService { | |||
| client: FollowerClient, | |||
| } | |||
|
|
|||
| #[async_trait] | |||
| pub trait FollowerServiceTrait { | |||
There was a problem hiding this comment.
Rename to OwnerResolver. I prefer traits to be named after the thing they do and not have "Trait" in their name
rewards/src/follower/client.rs
Outdated
| async fn find_owner( | ||
| &mut self, | ||
| address: &PublicKey, | ||
| test_owner: Option<PublicKey>, |
There was a problem hiding this comment.
While this is a neat trick, I think the right approach would be to have a different imp for this trait that is the Test version of the OwnerResolver right?
There was a problem hiding this comment.
Yeah, I much prefer the way you did it. I'll remember it from now on.
rewards/src/subnetwork_rewards.rs
Outdated
| @@ -167,20 +168,17 @@ pub async fn construct_rewards( | |||
| counter: Counter, | |||
| model: Model, | |||
| emitted: Emission, | |||
| default_owner: Option<PublicKey>, | |||
There was a problem hiding this comment.
so why are you not passing in the OwnerResolver here?
|
|
||
| let mut reward_acc = 0; | ||
| let mut reward_acc = dec!(0); |
There was a problem hiding this comment.
I think I see what you did here.. why aren't you using Mobile here instead of a new Decimal with a 1 multiplier? To avoid the Option? looks cleaner I think?
There was a problem hiding this comment.
Yeah, also, I didn't really want to impl Div and AddAssign for Mobile. The u64 does get constructed from Mobile once the rewards have been accumulated so it should preserve the 8 decimal precision afaict. Will try to test it more.
rewards/src/subnetwork_rewards.rs
Outdated
|
|
||
| let (txn, txn_hash_str) = construct_txn(&loaded_kp, SubnetworkRewards(rewards), 1000, 1010) | ||
| // TODO: read key from some test env | ||
| let kp = load_from_file("/tmp/reward_server_keys").expect("unable to load keypair"); |
There was a problem hiding this comment.
inline them as bytes or using the macros in the base64 crate?
Don’t try to work around the trait with default values.. Introduce a FixedOwnerResolver for test cases
simpler fetch commands
- Fix unnest query - Fix parameter binding for update_all query - Stick to lowercase for migrations
Summary
This PR has two major changes:
It introduces
async-traitwith potentially questionable usage forfind_ownerfor follower_service. The reason is that async closures are not part of stable rust, just yet. This is more of a hacky workaround to allow setting adefault_ownerand just ignore looking upfollower_servicefor testing.This fixes the emitted rewards in
construct_rewardsto correct evaluate the bones. We were sendingMobiledirectly instead of re-converting it back to bones.TODO