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 initial support for ParaTimes #156

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

lvshaoping007
Copy link
Contributor

1,add Paratime Deposit
2,fix #25

@lvshaoping007
Copy link
Contributor Author

i fix some spell check , but still have paratime Paratime and emera and more that i have not fix . can you give some suggestion

@lukaw3d
Copy link
Member

lukaw3d commented Nov 15, 2021

@lvshaoping007 i fix some spell check , but still have paratime Paratime and emera and more that i have not fix . can you give some suggestion

You can add those exceptions to

# List of words that are always considered correct.
words:
- oasisprotocol
- oasisscan

Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

I've played around with this locally, and depositing works.

I'd like to have us merge this.

Other collaborators here please review too.

@lvshaoping007
Copy link
Contributor Author

#157
add allowance dynamic process . when current allowance is greater than deposit amount . does not need setAllowance again

@pro-wh
Copy link
Contributor

pro-wh commented Nov 16, 2021

I discussed with @tjanez and the rest of our team. We'd like to squash some of the commits and merge.

@pro-wh
Copy link
Contributor

pro-wh commented Nov 16, 2021

I'll be squashing the first 10 commits--"add paratime tab" through "add cspell word." I appreciate that you had split up several additions in the first few commits, but the later fixes need to be integrated into those, and those don't apply cleanly to the individual earlier commits.

@pro-wh
Copy link
Contributor

pro-wh commented Nov 16, 2021

Removed xlink from the new SVG files. @tjanez do you want to do anything else before merging? I've filed several issues corresponding to things we'd like to do after merging.

@pro-wh
Copy link
Contributor

pro-wh commented Nov 17, 2021

Update: @tjanez will do a pass of review and then merge in the coming days. We're almost there folks!

@tjanez tjanez changed the title Paratime support Add initial support for ParaTimes Nov 19, 2021
Make getSubmitStatus() throw error if StakingAllow transaction doesn't
return code: 0.
@tjanez tjanez merged commit 33f4c7e into oasisprotocol:master Nov 19, 2021
* set deposit
* @param {*} params
*/
setDeposit= (params)=>{
Copy link
Member

Choose a reason for hiding this comment

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

@lvshaoping007 Would these be more correct function names?

APIService.setDeposit   // setAllowanceAndDepositToConsensusAccount?
  oasis.staking.allowWrapper()
    amount = allowanceDifference
  APIService.submitTxBody onBroadcastEnd
  APIService.onBroadcastEnd    // depositToConsensusAccount?
    APIService.submitRuntimeBody   // submitDepositToConsensusAccountTx?
      APIService.buildRuntimeTxBody   // buildDepositToConsensusAccountTx?
        oasisRT.consensusAccounts.Wrapper.callDeposit()
          amount = DEPOSIT_AMOUNT
      submitTx
      APIService.checkRuntimeTxStatus   // createNotificationAfterRuntimeTxSucceeds?
        extension.notifications.create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will fix this in next PR , but what does this means , should this be fixed ?

oasis.staking.allowWrapper()
    amount = allowanceDifference
    
oasisRT.consensusAccounts.Wrapper.callDeposit()
          amount = DEPOSIT_AMOUNT
      submitTx

Copy link
Contributor

Choose a reason for hiding this comment

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

I think those lines are included for context, and the names used there are fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

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.

Notification click listener doesn't unsubscribe
4 participants