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

Refactor to help integrate proposer in staging #578

Merged
merged 29 commits into from
Mar 25, 2022
Merged

Conversation

druiz0992
Copy link
Contributor

@druiz0992 druiz0992 commented Mar 13, 2022

Changes to help integrate new proposer in staging/prod. It shouldn't have any effect on dev:

To test changes on dev, run ping-pong test.

Closes #576

@druiz0992 druiz0992 added DNM Do not merge and removed DNM Do not merge labels Mar 13, 2022
ChaitanyaKonda
ChaitanyaKonda previously approved these changes Mar 14, 2022
@ChaitanyaKonda ChaitanyaKonda added the One more approval needed One reviewer has approved this PR but another is needed label Mar 14, 2022
@druiz0992 druiz0992 added the DNM Do not merge label Mar 16, 2022
@druiz0992
Copy link
Contributor Author

There has been some changes requested by @IlyasRidhuan to include an endpoint to get contract addresses from the proposer for the wallet. I will add these changes on this PR.

@druiz0992 druiz0992 temporarily deployed to AWS March 16, 2022 22:09 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 16, 2022 22:24 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 17, 2022 06:42 Inactive
@druiz0992 druiz0992 removed the DNM Do not merge label Mar 17, 2022
@druiz0992 druiz0992 temporarily deployed to AWS March 17, 2022 19:28 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 17, 2022 19:43 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 17, 2022 22:46 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 18, 2022 07:43 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 18, 2022 09:32 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 19, 2022 10:37 Inactive
@druiz0992 druiz0992 removed the DNM Do not merge label Mar 19, 2022
@druiz0992 druiz0992 temporarily deployed to AWS March 19, 2022 22:48 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 20, 2022 08:27 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 20, 2022 19:26 Inactive
@druiz0992 druiz0992 temporarily deployed to AWS March 20, 2022 21:05 Inactive
@ChaitanyaKonda ChaitanyaKonda dismissed their stale review March 21, 2022 11:09

Outdated because of new code

@druiz0992 druiz0992 temporarily deployed to AWS March 21, 2022 12:58 Inactive
const nf3 = new Nf3(signingKeys.proposer1, environment);
await nf3.init(mnemonics.proposer);

const nf3 = Nf3Instance(signingKeys.proposer1, environment);
Copy link
Contributor

Choose a reason for hiding this comment

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

If Nf3Instance is a wrapper around SDK calls, then we should be consistent and use this for the following lines too

  • await nf3.init(undefined, 'optimist');
  • await nf3.registerProposer(environment.proposerBaseUrl);

If not, we can remove Nf3Instance wrapper entirely and use nf3 from const nf3 = new Nf3(signingKeys.proposer1, environment); for all the calls to SDK too.

Copy link
Contributor Author

@druiz0992 druiz0992 Mar 21, 2022

Choose a reason for hiding this comment

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

The idea of this wrapper was to store a copy of the nf3 instance so that functions in route modules could access it. I did not intended it to be a real wrapper. But I followed your recommendation and encapsulated all functions. Done

Copy link
Contributor

@ChaitanyaKonda ChaitanyaKonda left a comment

Choose a reason for hiding this comment

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

Changes requested

@ChaitanyaKonda ChaitanyaKonda added Changes requested and removed One more approval needed One reviewer has approved this PR but another is needed labels Mar 21, 2022
@druiz0992 druiz0992 temporarily deployed to AWS March 21, 2022 17:53 Inactive
@ChaitanyaKonda ChaitanyaKonda merged commit 1861375 into master Mar 25, 2022
@ChaitanyaKonda ChaitanyaKonda deleted the david/staging3 branch March 25, 2022 10:20
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.

Merge proposer that registers URL to prod
4 participants