Skip to content

Add collateral address check in join federation service#720

Merged
fassadlr merged 2 commits intostratisproject:release/1.1.0.0from
fassadlr:colcheck
Sep 27, 2021
Merged

Add collateral address check in join federation service#720
fassadlr merged 2 commits intostratisproject:release/1.1.0.0from
fassadlr:colcheck

Conversation

@fassadlr
Copy link
Contributor

@fassadlr fassadlr commented Sep 27, 2021

No description provided.

@fassadlr fassadlr requested a review from quantumagi September 27, 2021 10:31
quantumagi
quantumagi approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@quantumagi quantumagi left a comment

Choose a reason for hiding this comment

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

LGTM. The only thing that can go wrong is if the member was previously registered and the node is in IBD. An error may arise due to previously being a member with that address. This can be alleviated by replacing IsCollateralAddressRegistered with CollateralAddressOwner and only raising an error if a different owner is detected.

@fassadlr
Copy link
Contributor Author

fassadlr commented Sep 27, 2021 •
edited
Loading

LGTM. The only thing that can go wrong is if the member was previously registered and the node is in IBD. An error may arise due to previously being a member with that address. This can be alleviated by replacing IsCollateralAddressRegistered with CollateralAddressOwner and only raising an error if a different owner is detected.

Perhaps we should add an IBD check in the service?

@quantumagi
Copy link
Contributor

quantumagi commented Sep 27, 2021 •
edited
Loading

Perhaps we should add an IBD check in the service?

Its another restriction/pain in what the user can do and when but maybe justified...

@fassadlr fassadlr merged commit cdea647 into stratisproject:release/1.1.0.0 Sep 27, 2021
@fassadlr fassadlr deleted the colcheck branch September 27, 2021 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@quantumagi quantumagi quantumagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fassadlr @quantumagi