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 proposedByDelegate to multisig mapping #2062

Merged
merged 4 commits into from
Oct 25, 2024
Merged

Conversation

iamacook
Copy link
Member

Summary

A new proposedByDelegate field has been added to multisig transactions on the Transaction Service. This is also needed by the client for delegate integration.

This adds the new field to the relevant validation schema and maps it accordingly.

Changes

  • Add property to relevant schemas/builder
  • Propagate property to mappings
  • Add/update tests accordingly

@iamacook iamacook self-assigned this Oct 25, 2024
@iamacook
Copy link
Member Author

iamacook commented Oct 25, 2024

I'll keep this as a draft until the Transaction Service is released. cc @Uxio0 @falvaradorodriguez

This is now on staging.

@iamacook iamacook marked this pull request as ready for review October 25, 2024 11:49
@iamacook iamacook requested a review from a team as a code owner October 25, 2024 11:49
Comment on lines 47 to 48
.with('proposer', delegate)
.with('proposedByDelegate', delegate)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think this wouldn't have any impact on the tests, but to make the instances generated by this builder more accurate: I think the proposer and the proposedByDelegate can't share the same address, right? Otherwise the transaction wouldn't be proposed by a delegate but by the owner themself and therefore proposedByDelegate would be null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I adjusted it to have a proposer and a null proposedByDelegate to keep it the same as s in f18756f, as well as adding a specific test in 18d8925.

@@ -246,4 +249,32 @@ describe('MultisigTransactionExecutionDetails mapper (Unit)', () => {
}),
);
});
it('should return a MultisigExecutionDetails object with no proposedByDelegate if not present', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Super nitpicky: missing newline just before this test case 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in f18756f.

@iamacook iamacook merged commit 1f7af10 into main Oct 25, 2024
20 checks passed
@iamacook iamacook deleted the proposed-by-delegate branch October 25, 2024 15:51
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