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

fix: send exn to actual recipient only #273

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

iFergal
Copy link
Contributor

@iFergal iFergal commented Aug 27, 2024

exn messages have a specific recipient (rp field).

Right now for multi-sig:

  • creates first exn message with rp field of first member
  • this message is sent to all members
  • returns early (bug in itself)

This change will just send the specific message to the specific member, and not return early.

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.93%. Comparing base (ce3614c) to head (02f178a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #273   +/-   ##
=======================================
  Coverage   83.93%   83.93%           
=======================================
  Files          48       48           
  Lines        4257     4259    +2     
  Branches     1051     1038   -13     
=======================================
+ Hits         3573     3575    +2     
+ Misses        680      656   -24     
- Partials        4       28   +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iFergal iFergal marked this pull request as draft August 27, 2024 11:40
@iFergal
Copy link
Contributor Author

iFergal commented Aug 27, 2024

ok seems to have broken 1 test, interesting.. checking

edit - ok, yes, forgot i need remove this return statement

@iFergal iFergal marked this pull request as ready for review August 27, 2024 12:48
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes here for audit check on pipeline

@@ -78,24 +78,19 @@ export class Exchanges {
payload: Dict<any>,
embeds: Dict<any>,
recipients: string[]
): Promise<any> {
for (const recipient of recipients) {
): Promise<any[]> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should get proper types here instead of any - don't have capacity right now but will try to sweep things in coming weeks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, this actually now returns an array of Promise. So more correct would be Promise<any>[].

However, a better option could be to change this method to have one recipient and adjust from the call site accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, it was right and i incorrectly refactored and TS didnt pick it up due to any - will fix.

But not so sure on changing the method to only have one recipient as this brings a lot of for loops at the call sites, which will make every wallet/app using multi-sig need to refactor everywhere.

Copy link
Contributor Author

@iFergal iFergal Aug 27, 2024

Choose a reason for hiding this comment

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

and i broke tests again - weird, seems adding the end role doesn't like these changes

@iFergal iFergal marked this pull request as draft August 27, 2024 16:01
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.

2 participants