Skip to content
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

add flow-edges from users with trusted tokens to organizations #40

Closed
wants to merge 7 commits into from

Conversation

jaensen
Copy link
Contributor

@jaensen jaensen commented May 30, 2023

No description provided.

@jaensen jaensen requested a review from chriseth May 30, 2023 18:28
if balance == &U256::from(0) {
continue;
}
organization_accepted_tokens.get(token).map(|organizations| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this compile? The function passed to map does not return anything.
Maybe if let Some(organizations) = rganization_accepted_tokens.get(token) {?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or use something like flat_map and for_each

Copy link
Contributor Author

Choose a reason for hiding this comment

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

organization_accepted_tokens.get(token) returns an Option<HashSet<Address>> so the method used is https://doc.rust-lang.org/std/option/enum.Option.html#method.map and not https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.map

@chriseth
Copy link
Collaborator

Does it work? :)


// Find all safes that have a non-zero balance of tokens that are accepted by an organization
for (user, safe) in &self.safes {
for (token, balance) in &safe.balances {
Copy link
Member

Choose a reason for hiding this comment

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

Meaning of balance.token
It's not clear to me what the token attribute of an Edge represents. Is it a token address, or a user address (i.e. the owner of the token)? Compare these 2 cases, where in the first it is assumed that the balance contains the token address, and in the second the token attribute of the balance is supposed to contain the token owner address:

I think the problem lies in the confusion created by the edge.token which we are using to store the token owner address. Correct me if I'm wrong. I suggest changing the name of the edge.token attribute to edge.token_owner once and for all.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we create a test image to update the pathfinder in production @jaensen ?

Copy link
Member

Choose a reason for hiding this comment

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

In my PR (#43), I assumed that balance.token is a token address and not a token's owner address. So the issue reported in my comment would be fixed there.

This was referenced Jul 24, 2023
@JacqueGM
Copy link
Contributor

already added in the only-server

@JacqueGM JacqueGM closed this Aug 24, 2023
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.

4 participants