feature: add auto restore functionality for contract client #974

BlaineHeffron · 2024-05-28T15:10:58Z

Adds the ability to set restore to true in contract method invocation options. This will utilize the signTransaction method to sign a restoreFootprint operation when found that it is needed during a simulation.

BlaineHeffron · 2024-05-28T15:23:01Z

A couple of interesting things I found:

At first I wanted to reuse the Account object so we don't query the server a second time for it. The problem I found was that the sequence number increments the first time the AssembledTransaction is built, and since that method call doesn't go through it is then off by 1 for the restoreFootprint operation. If I had a way to decrement the sequence number I could avoid this problem and reuse the Account object.
I was having trouble testing the case where the actual contract wasm hash footprint / contract wasm was expired. The way I was testing was by setting the minimum expire ledgers to some low number (around 300) in the quickstart config. Since persistent and instance storage use this same number, it would seem that the persistent storage and instance storage (which would presumably include the wasm hash) would expire at the same time. It seemed that regardless of what had expired, using the restorePreamble to construct the restoreFootprint operation always worked. In other words, I never needed to perform the actual restore of the contract wasm hash / contract wasm footprints. I did write code to do that in case its needed, and I can add that as a method if it is needed. I just wasn't able to find a test case where it was.

chadoh

This is gonna be so great! Various notes on the interface below. Really curious to hear from @Shaptic on your questions above.

src/contract/assembled_transaction.ts

test/unit/server/soroban/assembled_transaction_test.js

chadoh · 2024-05-29T17:58:54Z

test/unit/server/soroban/assembled_transaction_test.js

+      updateTimeout: false
+    }))
+    .then((result) => {
+      expect(result.getTransactionResponse.status).to.equal(rpc.Api.GetTransactionStatus.SUCCESS);


Do you find this test satisfying? Is there a meaningful way it can fail? Seems sorta like "if I make f(x) always return 1 then it will return 1".

Is there a better test we could write, that exercises real functionality? Like our e2e tests do?

I agree, its not the most satisfying test. However, I'm not sure how to do something like the e2e tests because you can't force a restore via any parameters. It has to be done at the server level (changing the minimum TTL) which would still require you to wait x amount of time, or if you put it too low it breaks functionality. So the only way I coudl think to write a test was to mock the response. What I could do is a better pattern match for the request, I was having trouble getting it to match exactly what the request is but its possible - just would take more detailed reconstruction of the specific transaction objects.

Yeah unfortunately testing restores is difficult by default at all levels of the stack. We'd have to have a custom rpc+core in the e2e tests that speeds up archiving :/

which makes me wonder: maybe we can run e2e tests only in CI and use a custom quickstart image?

As we discussed on the call this week, we will add the ability to modify the minimum TTL as a quickstart variable or some preset config which we can then use for an e2e test of this. For now I suppose the current test is okay?

src/contract/assembled_transaction.ts

src/contract/sent_transaction.ts

Shaptic · 2024-05-30T21:41:04Z

The problem I found was that the sequence number increments the first time the AssembledTransaction is built, and since that method call doesn't go through it is then off by 1 for the restoreFootprint operation.

Yeahh this has been a long-standing issue with the Account object. That's why TransactionBuilder.cloneFrom() does this janky -1n workaround. We can definitely make changes to that interface and its implementations, though. See also stellar/js-stellar-base#574 and stellar/js-stellar-base#436 for more stuff related to this. I'm definitely for it!

Shaptic

We'll have to release this in a minor release so that I can tag v12 stable today with no changes from the RC. This'll be better for the CLI release train too, I think.

src/contract/assembled_transaction.ts

src/contract/sent_transaction.ts

Shaptic · 2024-05-30T21:49:17Z

test/unit/server/soroban/assembled_transaction_test.js

+      updateTimeout: false
+    }))
+    .then((result) => {
+      expect(result.getTransactionResponse.status).to.equal(rpc.Api.GetTransactionStatus.SUCCESS);


Yeah unfortunately testing restores is difficult by default at all levels of the stack. We'd have to have a custom rpc+core in the e2e tests that speeds up archiving :/

Shaptic · 2024-05-30T21:49:47Z

test/unit/server/soroban/assembled_transaction_test.js

+      updateTimeout: false
+    }))
+    .then((result) => {
+      expect(result.getTransactionResponse.status).to.equal(rpc.Api.GetTransactionStatus.SUCCESS);


which makes me wonder: maybe we can run e2e tests only in CI and use a custom quickstart image?

src/contract/assembled_transaction.ts

fix name Co-authored-by: Chad Ostrowski <[email protected]>

better description Co-authored-by: Chad Ostrowski <[email protected]>

Better auto restore failure message. Co-authored-by: Chad Ostrowski <[email protected]>

BlaineHeffron · 2024-06-05T22:21:25Z

All comments are addressed except regarding the test. I think we can use this test for now until we have a way of using a quickstart image with a short minimum TTL that would be suitable for this type of e2e test. Changelog has been added.

chadoh

Awesome work here, thanks @BlaineHeffron!

Shaptic · 2024-06-13T16:27:02Z

@BlaineHeffron if you can resolve the conflicts I can merge this asap!

chadoh · 2024-06-13T20:22:44Z

@BlaineHeffron is out today! I resolved the conflicts but didn't have permission to push to Blaine's fork, so I made a new PR, if you want to merge that one and close this one, @Shaptic! #991

chadoh suggested changes May 29, 2024

View reviewed changes

chadoh reviewed May 30, 2024

View reviewed changes

src/contract/assembled_transaction.ts Outdated Show resolved Hide resolved

chadoh reviewed May 30, 2024

View reviewed changes

src/contract/sent_transaction.ts Outdated Show resolved Hide resolved

Shaptic reviewed May 30, 2024

View reviewed changes

BlaineHeffron and others added 9 commits June 5, 2024 18:03

add auto restore functionality for contract client

ef8a550

Update test/unit/server/soroban/assembled_transaction_test.js

532199d

fix name Co-authored-by: Chad Ostrowski <[email protected]>

Update test/unit/server/soroban/assembled_transaction_test.js

63a0225

better description Co-authored-by: Chad Ostrowski <[email protected]>

Update src/contract/assembled_transaction.ts

01e1f01

Better auto restore failure message. Co-authored-by: Chad Ostrowski <[email protected]>

extract getAccount to utils

b0ba301

TSDoc format

de6e88f

make account optional for restoreFootprint

06c19ba

remove bald booleans

f7672ef

cleanup, remove updateTimeout workaround, dont rebuild sorobandata

ae4db84

BlaineHeffron force-pushed the feat/AutoRestore branch from 523a526 to ae4db84 Compare June 5, 2024 22:03

BlaineHeffron added 3 commits June 5, 2024 18:16

add changelog entry for auto restore functionality

9ba6929

remove comment

e82c81f

remove unused var

fc48b50

BlaineHeffron added 2 commits June 5, 2024 18:27

simpler wording

a3b773d

fixed position of changelog entry

664bb83

BlaineHeffron requested review from Shaptic and chadoh June 5, 2024 23:05

chadoh approved these changes Jun 6, 2024

View reviewed changes

chadoh mentioned this pull request Jun 13, 2024

feature: add auto restore functionality for contract client #991

Merged

Shaptic closed this Jun 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add auto restore functionality for contract client #974

feature: add auto restore functionality for contract client #974

BlaineHeffron commented May 28, 2024

BlaineHeffron commented May 28, 2024

chadoh left a comment

chadoh May 29, 2024

BlaineHeffron May 29, 2024

Shaptic May 30, 2024

Shaptic May 30, 2024

BlaineHeffron Jun 5, 2024

Shaptic commented May 30, 2024

Shaptic left a comment

Shaptic May 30, 2024

Shaptic May 30, 2024

BlaineHeffron commented Jun 5, 2024

chadoh left a comment

Shaptic commented Jun 13, 2024

chadoh commented Jun 13, 2024

feature: add auto restore functionality for contract client #974

feature: add auto restore functionality for contract client #974

Conversation

BlaineHeffron commented May 28, 2024

BlaineHeffron commented May 28, 2024

chadoh left a comment

Choose a reason for hiding this comment

chadoh May 29, 2024

Choose a reason for hiding this comment

BlaineHeffron May 29, 2024

Choose a reason for hiding this comment

Shaptic May 30, 2024

Choose a reason for hiding this comment

Shaptic May 30, 2024

Choose a reason for hiding this comment

BlaineHeffron Jun 5, 2024

Choose a reason for hiding this comment

Shaptic commented May 30, 2024

Shaptic left a comment

Choose a reason for hiding this comment

Shaptic May 30, 2024

Choose a reason for hiding this comment

Shaptic May 30, 2024

Choose a reason for hiding this comment

BlaineHeffron commented Jun 5, 2024

chadoh left a comment

Choose a reason for hiding this comment

Shaptic commented Jun 13, 2024

chadoh commented Jun 13, 2024