Skip to content

GET total_deposited + spender_leaf route completed + tests #416

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

Merged
merged 29 commits into from
Aug 26, 2021

Conversation

simzzz
Copy link
Contributor

@simzzz simzzz commented Jul 21, 2021

Questions :

  • Should we add a different Error struct for the create_spendable_document() helper function like we did for the campaign routes?
  • What will happen if an error is thrown after the new Spendable is inserted in DB (ex. during total_spent calculations) and should we worry about it?
  • adapter.get_deposit() returns a deposit that uses BigNum instead of the UnifiedNum we need for spendable. How should we deal with that?
  • NewStateuses BalancesMap which uses BigNum instead of UnifiedMap which uses UnifiedNum. How should we deal with that?

@simzzz simzzz requested review from samparsky and elpiel and removed request for samparsky July 26, 2021 12:02
@elpiel
Copy link
Member

elpiel commented Aug 4, 2021

Q: > adapter.get_deposit() returns a deposit that uses BigNum instead of the UnifiedNum we need for spendable. How should we deal with that?
A: Using the Deposit asset of the channel we should convert the BigNum to UnifiedNum based on the precision of the token.

Q: > NewState uses BalancesMap which uses BigNum instead of UnifiedMap which uses UnifiedNum. How should we deal with that?
A: It should use UnifiedNum. It just hasn't been changed yet.

I will answer the other 2 questions once I review the PR.

samparsky
samparsky previously approved these changes Aug 5, 2021
@elpiel
Copy link
Member

elpiel commented Aug 6, 2021

Should we add a different Error struct for the create_spendable_document() helper function like we did for the campaign routes?
You would probably need a new Error struct. There's probably a nicer way for those errors instead of creating new structs for Error everywhere but for the time being create a new struct.
It will hold the A::AdapterError and PoolError.

What will happen if an error is thrown after the new Spendable is inserted in DB (ex. during total_spent calculations) and should we worry about it?

As for the total_spent there won't be an error as I mentioned in the review. We only return the new_state.balances.spenders.get(spender) there

Base automatically changed from v5-modify-submit-campaigns to issue-382-campaign-routes August 9, 2021 14:24
@simzzz simzzz changed the base branch from issue-382-campaign-routes to aip-61-adex-v5 August 11, 2021 16:06
@elpiel elpiel dismissed samparsky’s stale review August 12, 2021 09:42

Just approved, the PR was not ready with multiple TODOs

Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

Could you please add camelCase to both structs SpenderResponse & SpenderLeaf.
It's missing now

@elpiel
Copy link
Member

elpiel commented Aug 16, 2021

  • Also separate the Spender (to spender.rs) and the make the SpenderResponse { spender: Spender }

  • For spender/all then we can use: AllSpendersReponse { spenders: HashMap<Address, Spender> } but later we will add pagination, so more fields will be added to the struct

  • Spender { total_deposited, spender_leaf: SpenderLeaf } - please change total to total_deposited

Please note that: we have the address in the route /spender/:address, this is not true for the spender/all route, so we need include the Address (i.e. using the HashMap<Address, Spender>)

Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

Make sure that clippy passes the checks & run rustfmt

@elpiel elpiel linked an issue Aug 25, 2021 that may be closed by this pull request
4 tasks
@simzzz simzzz merged commit 71af607 into aip-61-adex-v5 Aug 26, 2021
@elpiel elpiel deleted the issue-391-get-spender-info branch September 28, 2021 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AIP #61 v5: GET /channel/:id/spender/:addr
3 participants