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

feat(sdk-coin-atom): add sendMany support #3965

Merged
merged 1 commit into from
Oct 10, 2023
Merged

feat(sdk-coin-atom): add sendMany support #3965

merged 1 commit into from
Oct 10, 2023

Conversation

dpkjnr
Copy link
Contributor

@dpkjnr dpkjnr commented Oct 6, 2023

Cosmos side chains natively support transactions with multiple recipients. This PR adds test cases to verify multiple recipient transaction building and fixes explain transaction code to generate input and outputs for all recipients.
Added tests only for Atom and Tia as Atom doesn't extend common cosmos classes, and adding tests for any one of other coins that extend cosmos base classes would suffice
Ticket: WIN-17

@dpkjnr dpkjnr marked this pull request as ready for review October 9, 2023 04:55
@dpkjnr dpkjnr requested review from a team as code owners October 9, 2023 04:55
@@ -268,7 +268,7 @@ describe('Zeta', function () {
amount: 'UNAVAILABLE',
},
],
outputAmount: 'UNAVAILABLE',
Copy link
Contributor

Choose a reason for hiding this comment

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

any specific reason why we changed this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see any usage of it and made is same as atom


const tx = await txBuilder.build();
const json = await (await txBuilder.build()).toJson();
should.equal(tx.type, TransactionType.Send);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd need this test for the other supported transaction types too

Copy link
Contributor Author

@dpkjnr dpkjnr Oct 9, 2023

Choose a reason for hiding this comment

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

Added for celestia

amount: UNAVAILABLE_TEXT,
},
];
outputAmount = UNAVAILABLE_TEXT;
Copy link
Contributor

Choose a reason for hiding this comment

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

was this intentionally removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, to make is similar to atom. I also checked that it is not being used anywhere

amount: [
{
denom: 'uatom',
amount: '500',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we use a txn with different amounts for the two recipients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doing it for delegate and undelegate txns

@@ -209,6 +232,31 @@ describe('ATOM', function () {
});
});

it('should explain sendMany transfer transaction', async function () {
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 add one testcase for sendMany delegate and undelegate transfer type also.

Copy link
Contributor

Choose a reason for hiding this comment

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

same for any abstract-cosmos coin also.

amount: [
{
denom: 'utia',
amount: '100000',
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, can we use different amounts.

Copy link
Contributor

@mullapudipruthvik mullapudipruthvik left a comment

Choose a reason for hiding this comment

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

LGTM @DinshawKothari or @Vijay-Jagannathan can be final approver

@dpkjnr dpkjnr merged commit e82d6a7 into master Oct 10, 2023
6 checks passed
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