clients/go: Bump oasis-core to 23.0 #278

matevz · 2024-02-21T11:30:32Z

This PR:

Bumps clients/go to use oasis-core 23.x types.
removes redundant data types which are already part of the oasis-sdk/client-sdk/go
removes SendTransaction reimplementation
fixes signed query implementation
implements encrypted and signed eth_estimateGas calls
adds test for PackSignedCall()

netlify · 2024-02-21T11:30:54Z

✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name	Link
🔨 Latest commit	`b615766`
🔍 Latest deploy log	https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/65eae49b5d9c2d000834118f

clients/go/compat.go

clients/go/cipher.go

clients/go/cipher_test.go

clients/go/go.mod

aefhm

Makeshift e2e script in #206

clients/go/cipher.go

clients/go/compat.go

kostko

This thing still needs some refactoring.

clients/go/compat.go

kostko · 2024-02-28T12:52:22Z

This current API seems confusing and error-prone. It looks like it has two hooks to manipulate transactions:

It can generate a TransactOpts instance with a wrapped signer that also performs encryption.
It then also provides a wrapped SendTransaction which performs mangling, but in this case it is already too late in the pipeline to actually return the modified transaction! So in case this second fallback (e.g. TransactOpts not properly set, instead going through SendTransaction directly) is used the transaction that has actually been sent to Sapphire cannot be retrieved by the developer. Then there is additional hacky logic to detect whether a transaction was previously mangled by the wrapped signer so that it is not wrapped again.

I think it would be better if SendTransaction was not overriden and in case you want encrypted transactions you either need to encrypt them manually or use the Transactor.

clients/go/README.md

clients/go/compat.go

kostko

As mentioned above, I think we should remove the hacky SendTransaction behavior unless there is a good reason to keep it.

clients/go/README.md

clients/go/compat.go

clients/go/README.md

clients/go/compat.go

aefhm · 2024-03-12T17:23:49Z

I suppose this'll effect a minor version bump?

aefhm · 2024-03-12T17:28:32Z

clients/go/compat.go

-	dataPack, err := NewDataPack(sign, chainID.Uint64(), msg.From[:], msg.To[:], 0, msg.GasPrice, msg.Value, msg.Data, leash)
+func PackSignedCall(msg ethereum.CallMsg, cipher Cipher, sign SignerFn, chainID big.Int, leash *evm.Leash) (*ethereum.CallMsg, error) {
+	if msg.Gas == 0 {
+		msg.Gas = DefaultGasLimit // Must be non-zero for signed calls.


So even 1 will work right? Just confirming as we don't use DefaultGasLimit otherwise.

1 will "work" in the sense that it will pass signature verification, but probably won't be enough to actually run anything useful.

👍 I thought this was overriding the the msg.Gas, but it is not.

matevz requested a review from peternose February 21, 2024 11:31

kostko reviewed Feb 21, 2024

View reviewed changes

clients/go/compat.go Outdated Show resolved Hide resolved

matevz force-pushed the matevz/clients/go/bump-oasis-core branch 3 times, most recently from 4e2787c to de3d537 Compare February 21, 2024 11:51

peternose reviewed Feb 21, 2024

View reviewed changes

clients/go/cipher.go Outdated Show resolved Hide resolved

clients/go/cipher.go Outdated Show resolved Hide resolved

clients/go/cipher_test.go Outdated Show resolved Hide resolved

aefhm reviewed Feb 21, 2024

View reviewed changes

clients/go/go.mod Outdated Show resolved Hide resolved

aefhm approved these changes Feb 21, 2024

View reviewed changes

matevz force-pushed the matevz/clients/go/bump-oasis-core branch from a17c3ee to 59cdfc7 Compare February 22, 2024 14:19

peternose reviewed Feb 22, 2024

View reviewed changes

clients/go/cipher.go Outdated Show resolved Hide resolved

clients/go/cipher.go Show resolved Hide resolved

clients/go/cipher.go Outdated Show resolved Hide resolved

clients/go/cipher.go Show resolved Hide resolved

aefhm reviewed Feb 23, 2024

View reviewed changes

clients/go/compat.go Outdated Show resolved Hide resolved

matevz force-pushed the matevz/clients/go/bump-oasis-core branch 6 times, most recently from 925c330 to 30e17d3 Compare February 27, 2024 16:56

kostko reviewed Feb 28, 2024

View reviewed changes

clients/go/compat.go Show resolved Hide resolved

matevz force-pushed the matevz/clients/go/bump-oasis-core branch 2 times, most recently from 61b1884 to 9166cc1 Compare February 28, 2024 17:34

kostko reviewed Feb 28, 2024

View reviewed changes

clients/go/README.md Outdated Show resolved Hide resolved

matevz force-pushed the matevz/clients/go/bump-oasis-core branch 6 times, most recently from 854292b to c74ef18 Compare March 1, 2024 13:03

kostko reviewed Mar 2, 2024

View reviewed changes

matevz force-pushed the matevz/clients/go/bump-oasis-core branch from c74ef18 to f2543be Compare March 4, 2024 15:35

matevz requested a review from kostko March 4, 2024 15:47

kostko reviewed Mar 4, 2024

View reviewed changes

matevz force-pushed the matevz/clients/go/bump-oasis-core branch 6 times, most recently from 9c7ade3 to 3115fee Compare March 5, 2024 15:16

kostko reviewed Mar 6, 2024

View reviewed changes

clients/go/README.md Outdated Show resolved Hide resolved

clients/go/compat.go Outdated Show resolved Hide resolved

matevz force-pushed the matevz/clients/go/bump-oasis-core branch 4 times, most recently from 2e4eed0 to 0e5924b Compare March 7, 2024 15:33

matevz requested a review from kostko March 7, 2024 15:35

kostko reviewed Mar 8, 2024

View reviewed changes

clients/go/README.md Outdated Show resolved Hide resolved

clients/go/README.md Show resolved Hide resolved

clients/go/compat.go Outdated Show resolved Hide resolved

clients/go/compat.go Outdated Show resolved Hide resolved

matevz added 3 commits March 8, 2024 11:11

clients/go: Bump oasis-core to 23.0

a9208ad

clients/go/README: WrapClient return parameter, style changes

e113b46

clients/go: Add support for Sapphire Localnet

b615766

matevz force-pushed the matevz/clients/go/bump-oasis-core branch from 0e5924b to b615766 Compare March 8, 2024 10:12

matevz requested a review from kostko March 8, 2024 16:18

kostko approved these changes Mar 8, 2024

View reviewed changes

matevz merged commit c45c136 into main Mar 11, 2024
15 checks passed

matevz deleted the matevz/clients/go/bump-oasis-core branch March 11, 2024 10:51

aefhm reviewed Mar 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clients/go: Bump oasis-core to 23.0 #278

clients/go: Bump oasis-core to 23.0 #278

matevz commented Feb 21, 2024 •

edited

Loading

netlify bot commented Feb 21, 2024 •

edited

Loading

aefhm left a comment

kostko left a comment

kostko commented Feb 28, 2024 •

edited

Loading

kostko left a comment

aefhm commented Mar 12, 2024

aefhm Mar 12, 2024

kostko Mar 12, 2024

aefhm Mar 13, 2024

clients/go: Bump oasis-core to 23.0 #278

clients/go: Bump oasis-core to 23.0 #278

Conversation

matevz commented Feb 21, 2024 • edited Loading

netlify bot commented Feb 21, 2024 • edited Loading

✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.

aefhm left a comment

Choose a reason for hiding this comment

kostko left a comment

Choose a reason for hiding this comment

kostko commented Feb 28, 2024 • edited Loading

kostko left a comment

Choose a reason for hiding this comment

aefhm commented Mar 12, 2024

aefhm Mar 12, 2024

Choose a reason for hiding this comment

kostko Mar 12, 2024

Choose a reason for hiding this comment

aefhm Mar 13, 2024

Choose a reason for hiding this comment

matevz commented Feb 21, 2024 •

edited

Loading

netlify bot commented Feb 21, 2024 •

edited

Loading

kostko commented Feb 28, 2024 •

edited

Loading