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

Relay validator set updates #1157

Merged
merged 10 commits into from
Feb 15, 2023

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Feb 14, 2023

Based on #1159.
Implements item 6) of #242.

@sug0 sug0 marked this pull request as draft February 14, 2023 16:30
@sug0 sug0 force-pushed the tiago/ethbridge/smart-contract-calls branch from 4f3a1ab to c583fbe Compare February 14, 2023 17:05
@sug0 sug0 marked this pull request as ready for review February 14, 2023 17:06
[u8; 32],
[u8; 32],
Vec<Signature>,
) = abi_decode_struct(encoded_proof);
Copy link
Member

Choose a reason for hiding this comment

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

I seems like we are doing lots of unnecessary rounds of serialization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the RPC method could return a non-abi encoded data response. right now it borsh serializes the abi encoding of the data. CLI commands requesting a proof are kind of obsolete now, but if we wanted to keep them anyway (so users could potentially implement shell scripts around this or smth?), they could be responsible for returning the ABI encoding of the data queried from our RPC endpoints

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to borsh serialize the raw struct and down the road, if some script needs abi encoding, it can call abi encoding on the raw struct itself. I don't think that's an unreasonable burden to put on people implementing their own scripts.

I just feel like we have a lot of artifact code that I would like to clear out. This damn repo is complicated enough as it is.

Copy link
Collaborator Author

@sug0 sug0 Feb 15, 2023

Choose a reason for hiding this comment

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

I feel the same. I'll work on this later though, right now it's out of the scope of this PR

EDIT: opened #1160

println!("{transf_result:?}");
}

// NOTE: there's a bug (or feature?!) in ethers, where
Copy link
Member

Choose a reason for hiding this comment

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

Probably a feature 😆

apps/src/lib/cli.rs Show resolved Hide resolved
@sug0 sug0 marked this pull request as draft February 15, 2023 10:12
@sug0 sug0 marked this pull request as ready for review February 15, 2023 11:13
@sug0 sug0 requested a review from batconjurer February 15, 2023 11:13
@sug0 sug0 merged commit fcd3382 into eth-bridge-integration Feb 15, 2023
@sug0 sug0 deleted the tiago/ethbridge/smart-contract-calls branch February 15, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants