Add method createRejectionTransaction #38
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
| * @param safeTransaction - The pending Safe transaction to be invalidated | ||
| * @returns The Safe transaction that invalidates the pending Safe transaction | ||
| */ | ||
| async rejectTransaction(safeTransaction: SafeTransaction): Promise<SafeTransaction> { |
There was a problem hiding this comment.
I wonder if the name should be createRejectionTransaction. This fits more to the existing createTransaction and in my opinion closer to what is happening.
There was a problem hiding this comment.
Maybe in the SDK, we'd also want API to be about replacing a transaction. In the UI it was simplified to "cancelation", but maybe in the SDK, we can be more realistic about what's happening behind the scenes. The API can be way simpler too: .replaceTransaction(nonce)
There was a problem hiding this comment.
I like createRejectionTransaction(nonce). Using the word "replace" is not my favourite option because it is true that we are replacing a transaction, but always with an empty transaction, not a custom one.
| @@ -197,21 +212,60 @@ describe('Transactions execution', () => { | |||
| .expect(safeInitialBalance.toString()) | |||
| .to.be.eq(safeFinalBalance.add(BigNumber.from(tx.data.value).toString())) | |||
| }) | |||
|
|
|||
| it('should fail if a transaction that was rejected is executed', async () => { | |||
There was a problem hiding this comment.
I found this test description hard to understand, I think something like should fail if you try to execute replaced transaction. Maybe I'm nitpicking, but I had to read the test to understand what's happening
There was a problem hiding this comment.
What do you think about should fail if the user tries to execute a transaction that was invalidated?
There was a problem hiding this comment.
I don't like it because we don't really use invalidated, if you replace it with rejected then it sounds good to me. The description I suggested is even simpler because it doesn't contain passive voice
germartinez
changed the title
Adds the method rejectTransaction that invalidates a pending Safe transaction.