Skip to content

Support "amend a pending transaction"#3045

Merged
Keith-CY merged 16 commits into
developfrom
fix-332
Mar 14, 2024
Merged

Support "amend a pending transaction"#3045
Keith-CY merged 16 commits into
developfrom
fix-332

Conversation

@devchenyan

@devchenyan devchenyan commented Feb 26, 2024

Copy link
Copy Markdown
Contributor

issue: Magickbase/neuron-public-issues#332

Screen-2024-02-27-160430.mp4

@devchenyan
devchenyan marked this pull request as ready for review February 27, 2024 07:37
@Danie0918

Copy link
Copy Markdown

@yanguoyu @homura Please have a review.

@homura homura left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if the AmendTransaction entity is necessary for this PR. The amendHash appears to be the same as the original transaction's hash on their feature, which makes me wonder if we can remove AmendTransaction.

To implement the amends transaction feature in Neuron, we would need to:

  1. Add an "amend" button for each pending transaction.
  2. Create an "amend" page that allows users to increase the transaction fee.
  3. Fetch the minimal RBF fee and use it as the default amend fee.
  4. Send the amended transaction.

In my opinion, the AmendTransaction is unnecessary for this feature and could be removed.

Comment thread packages/neuron-ui/src/components/AmendSend/hooks.ts Outdated
Comment thread packages/neuron-wallet/src/services/tx/failed-transaction.ts Outdated
Comment thread packages/neuron-ui/src/locales/es.json
Comment thread packages/neuron-ui/src/components/History/RowExtend.tsx
Comment thread packages/neuron-ui/src/components/AmendSend/index.tsx Outdated
Comment thread packages/neuron-ui/src/components/AmendSend/hooks.ts
Comment thread packages/neuron-wallet/src/services/transaction-sender.ts
@Danie0918
Danie0918 requested review from homura and yanguoyu March 4, 2024 02:50
@Danie0918

Copy link
Copy Markdown

@homura @yanguoyu Please have a review.

Comment thread packages/neuron-ui/src/components/AmendSend/index.tsx Outdated
Comment thread packages/neuron-ui/src/components/AmendSend/hooks.ts
@Danie0918

Copy link
Copy Markdown

@homura Please have a review.

@silySuper

Copy link
Copy Markdown

/package

@silySuper

Copy link
Copy Markdown

I can not see amend feature in this package for mac-arm 64:https://github.com/nervosnetwork/neuron/actions/runs/8169785200?pr=3045
截屏2024-03-07 15 04 55

@devchenyan

Copy link
Copy Markdown
Contributor Author

/package

Comment thread packages/neuron-ui/src/components/AmendSend/hooks.ts Outdated
@silySuper

Copy link
Copy Markdown

1.After amend twice(0.00000601 CKB-->0.00001502 CKB-->0.00002403 CKB ),2403 fee transaction will failed,lower fee will disappear.After a while 1502 fee transaction status shows confirming.

2024-03-11.15.45.02.mov

2.Send to address can not see whole in amend page.

截屏2024-03-11 15 28 48

3.Failed status can not see in history sometimes,but can see somtimes.Also as amend feature shows when transaction is pending status (data in 2024-03-11 is amend transation),Why?

截屏2024-03-11 15 53 46

@devchenyan

@Danie0918

Copy link
Copy Markdown

3.Failed status can not see in history sometimes,but can see somtimes.Also as amend feature shows when transaction is pending status (data in 2024-03-11 is amend transation),Why?

This feature is intended to solve the problem of pending transactions not being able to be packaged and therefore provides modification operations only in the pending state.

As for the failure state not being visible, it may be related to the requirement. #Magickbase/neuron-public-issues#332 (comment)

@silySuper

Copy link
Copy Markdown

4.sudt transaction amend will throw error.
截屏2024-03-11 16 26 20

@silySuper

silySuper commented Mar 11, 2024

Copy link
Copy Markdown

5.sudt amend page is not correct.In expect it should like sudt send page ,but actually is like this:

截屏2024-03-11 16 45 33

sudt page:

截屏2024-03-11 17 20 01

@silySuper

Copy link
Copy Markdown

6.ckb asset account receive lost amend feature.

2024-03-11.17.09.27.mov

@devchenyan

devchenyan commented Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

@silySuper

silySuper commented Mar 12, 2024

Copy link
Copy Markdown

All is same as before ,please check whether it is the testing package

@devchenyan

devchenyan commented Mar 12, 2024

Copy link
Copy Markdown
Contributor Author

@silySuper

Copy link
Copy Markdown

/package

@yanguoyu

yanguoyu commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

/package
Packaging for test is done in 8244834788. @yanguoyu

@silySuper

Copy link
Copy Markdown

1.verified
2.verified
3.verified
4.same as before
截屏2024-03-12 15 10 43
5.verified
6.not need to change

@silySuper

Copy link
Copy Markdown

7.amend page verify custom price will cause automatic return to home page.

2024-03-12.15.14.40.mov

@devchenyan

devchenyan commented Mar 12, 2024

Copy link
Copy Markdown
Contributor Author

/package
Packaging for test is done in 8253604787. @devchenyan

@silySuper

Copy link
Copy Markdown

4.verified
7.verified

@silySuper

Copy link
Copy Markdown

8.Amend transaction send pageshows abnormal when transaction sent by consuming cell(cell management)

image

9.In Nervos Dao ,amend trasaction type is changed to sent,in expect it should shows Nervos Dao
截屏2024-03-13 15 25 54

@devchenyan

devchenyan commented Mar 13, 2024

Copy link
Copy Markdown
Contributor Author

/package
Packaging for test is done in 8261934198. @devchenyan

@silySuper

Copy link
Copy Markdown

8.verified
9.verified

@devchenyan
devchenyan requested a review from silySuper March 14, 2024 02:26
@Keith-CY
Keith-CY merged commit 57b018e into develop Mar 14, 2024
@Keith-CY
Keith-CY deleted the fix-332 branch March 14, 2024 06:25
yanguoyu pushed a commit to yanguoyu/neuron that referenced this pull request Mar 14, 2024
Co-authored-by: Chen Yu <keithwhisper@gmail.com>
@Keith-CY Keith-CY mentioned this pull request Mar 14, 2024
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.

6 participants