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

update DOT sdk #3816

Merged
merged 1 commit into from
Aug 18, 2023
Merged

update DOT sdk #3816

merged 1 commit into from
Aug 18, 2023

Conversation

noel-bitgo
Copy link
Contributor

@noel-bitgo noel-bitgo commented Aug 17, 2023

update DOT library to remove controller optional field -> paritytech/txwrapper-core#309 and paritytech/substrate#14039

DOT is defaulting the controller address as the stash address

@socket-security
Copy link

socket-security bot commented Aug 17, 2023

@noel-bitgo noel-bitgo force-pushed the EA-1242 branch 2 times, most recently from 072dad4 to 08eb878 Compare August 17, 2023 16:44
Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

PR is not ready for review

@noel-bitgo noel-bitgo force-pushed the EA-1242 branch 9 times, most recently from fc7ed1e to aaf690a Compare August 17, 2023 21:40
@@ -179,7 +179,7 @@ export type StakeArgsPayeeRaw = { controller?: null; stash?: null; staked?: null
*/
export interface StakeArgs {
value: string;
controller?: { id: string };
controller: { id: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

Undocumented breaking change. Need a BREAKING CHANGE: in the commit footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually reverting the previous change a6f6a0d

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'll keep that in mind for future breaking changes

Copy link
Contributor

@OttoAllmendinger OttoAllmendinger left a comment

Choose a reason for hiding this comment

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

I have insufficient context to review this PR.

To flush it from my review queue, I am leaving this comment 🌊

This PR will still appear in the review queue of the other team members.

Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

Im lacking some context on the dot implementation, but from skimming the PR it lgtm. If you can get an approval from eth-alt or staking I'll approve this PR.

@noel-bitgo noel-bitgo merged commit ab845e3 into master Aug 18, 2023
5 of 7 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.

7 participants